-
Notifications
You must be signed in to change notification settings - Fork 105
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
gradient for cubic b-splines, and hessian for all b-splines #99
Conversation
cc @spencerlyon2 |
Hmm something is definitely off here. I'll take another look at my derivations and make sure the implementation matches . I liked my Mathematica notebook as I didn't actually do any derivations by hand, I just let Mathematica take derivatives and collect coefficients for me. I think the most likely place for an error in the math is at the very beginning when I derived the polynomial form of the cubic spline |
Yeah - that is definitely a great selling point for doing things like this in Mathematica. (An embarassingly large portion of the time it took me to implement this was spent taking derivatives of polynomials...) I would definitely be interested if there was some place online to share (and possibly actually execute) Mathematica notebooks, perhaps similar to JuliaBox or even nbviewer. |
Re: where the problem lies; it seems in the plots like it's mainly the boundary conditions that are off. The interior domain seems fine. |
Hmm that's true. I'm surprised by this because once I have the main polynomial I don't do any calculations by hand. Anyway, I'll double check and report back |
👍 |
Hmm, I'm not sure what the problem is here... Let's consider The polynomial for the interior is: This means that
Which is exactly the condition implemented here (actual implementation is found in quadratic.jl) The one thing I can see that is wrong is that we shouldn't have a type parameter over Any thoughts? |
``` | ||
|
||
""" | ||
function _build_woodbury_specs{T}(::Type{T}, n::Int, args::Tuple{Int, Int, Any}...) |
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.
This has been merged into Woodbury is now (unexported) WoodburyMatrices. sparse_factors
in the latest tagged release (0.1.4)
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.
See #104
Ah, that explains it. The lower boundary for |
Oh man... my mistake. Sorry about that. how do we know that it is at 1? |
Well, the underlying data is usually an array, with indices 1:N. Starting the interpolation at 1 is necessary to make I don't know if it's explicitly documented anywhere, but it should be implied in the latex docs (and by |
Ha! I found it! It's not the boundary conditions after all - although you did the derivations of boundary conditions with the wrong boundary in mind, the results are actually equivalent; you considered However, you copied one thing too many from the implementation of quadratic b-splines :) A quadratic b-spline is supported by three coefficients, and centering over them the boundaries end up at half-intervals. In other words, With With I'll patch |
Good catch. So with #102 is this fixed? |
Yep, should be. I'm going to spend some time on this later tonight (in UTC+1...), hopefully outputting some tests for this PR and other things, as well as tagging and releasing a new version. |
Sounds great, thanks. Let me know if you need any help! |
Sigh. After spending way more time than I should on troubleshooting something that in the end turned out to be equivalent to this:
I now have a working implementation of both |
Wow, great! Sympathies for the misguided bug hunt. I had a remarkably similar thing happen to me last week, and it cost me a lot of time, too. Why is it always the bugs in the test suite that are the worst to track down? (I guess there's a good answer to that, but still...) |
This is in preparation of removing this method entirely, in favor of the same function under another name now provided from the WoodburyMatrices package.
f65fbde
to
c77edcb
Compare
I haven't written a test suite for these implementations yet, but given the plots below I feel fairly confident that they are correct (there are some tests for the cubic gradient). Unfortunately, I'm going to have less time to spend on Julia over the coming weeks/months, so I want to make sure the work I've done so far this time isn't left hanging. My proposed plan of action is this:
(and, later, or by someone else than me)
Before I hit merge on this, I would like at least one other person to say that this is a sane plan, given that merging this in its current state would mean adding quite a lot of code that has basically no automated tests at all. Plots for sanity tests (generating script at the bottom):
|
Hey @tlycken sorry for the delay. Can you remind me which parts of this PR will introduce untested code? Otherwise I think your proposal sounds great |
I've only written a few sanity tests for I've also exported |
I'm taking @timholy's silence as agreement with the plan, and hitting the button on this :) |
gradient for cubic b-splines, and hessian for all b-splines
I'm super-excited about this, but just haven't had two halves-of-an-hour to rub together. Good that you hit merge. I will check this out soon, I promise. |
This is a WIP implementation for #94.
I still have to add tests, and the gradients aren't well-behaved for all boundary conditions. Not sure if the problem is with the gradients or (more likely) with the implementations of the boundary conditions in the cubic prefiltering functions.