-
Notifications
You must be signed in to change notification settings - Fork 114
Add regularization for BSpline #348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks interesting! To be merged, this also needs tests in addition to addressing/responding to the other comments below.
src/b-splines/prefiltering.jl
Outdated
sz = size(ret) | ||
first = true | ||
if ndims(ret) > 1 | ||
@warn "Smooth BSpline only available for Vectors, fallback to non-smooth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about restricting interpolate
to AbstractVector
when λ
and k
are supplied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work for higher dimensions too, it is just not implemented yet.
When implemented, it will be easier to change then.
src/b-splines/prefiltering.jl
Outdated
M, b = prefiltering_system(TWeights, eltype(TCoefs), sz[1], degree(it)) | ||
### TEST REGULARIZATION | ||
n = sz[1] | ||
Q = Matrix(diffop(TWeights, n, k)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have to enforce dense here? I'm wondering what happens on a large array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to transform to Dense matrix because I was getting an error running Cholesky with sparse matrices (see next line). I don't know enough about this type of optimization to solve it. But yes, there is no reason for it to be dense.
I added tests, but they are maybe too minimal. I have no idea what is important to check. If somebody can comment on it that would be great. |
This looks good to merge this now. Could we update the base branch so this can automatically merged? |
It would be good to try optimizing the code before (but I don't know enough to do it), like for instance @timholy 's comment about transforming to a dense Matrix, I think it could be avoided. |
Tests are passing here and this is mergeable, so I'm merging for v0.13.1 prospectively. Feel free to submit another PR for further optimization slated for v0.13.2 or earlier. |
Closes #254
Implements a regularized BSpline.
To implement a regularized spline on unequal-grid, the x-axis should be provided from the beginning, it cannot work with defining a ScaledSpline afterwards.
No tests for now and minimal documentation.