Conversation
Contributor
Author
|
The Travis failure reminded me that I had to patch ImageTransformations. So these tests won't pass until JuliaImages/ImageTransformations.jl#49 is merged and tagged. |
Member
|
Beautiful work! I don't see a single thing that should stand in the way of merging this. (Just to prove I read it, if you do have some good reason you need to fix/rebase/push again, you could use a space in Thank you so much for tackling this, this is a really spectacular contribution! |
Member
|
(Merge at will. I don't see a need to wait for ImageTransformations unless you want to be certain.) |
…d handling of SD, stop recentering and encourage users to center inputs, add tests
c575b30 to
1c738b1
Compare
Contributor
Author
|
Awesome, thanks for reviewing quickly! I'll merge and create an issue describing the aliasing issue from above so I don't forget. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This supersedes #81
Major changes:
Added
qd_translateandqd_affinefixed handling of the sample spacing matrix
SD: it must be updated between each pair of steps in a multi-step registration.Stop recentering the output transform in the qd_* family of registration functions and update docs to encourage users to call
centeredon inputsAdd tests, strengthen test criteria
Some 3D affine registration tests don't pass reliably so I have commented them out. I spent some time looking into this and my suspicion is that when optimizing full affine transforms the
mismatchobjective function is especially prone to local minima for scaled versions of the images. So typically QuadDIRECT jumps directly to the most extreme scaling in the search space, and sometimes it gets stuck there and never finds the better, more moderate scaling. Typically it expands (rather than contracts) themovingimage.I have a guess at why it tends to do this: expanding
movingmeans that there will be fewer pixels of overlap so the denominator of the mismatch will go down. Ideally the numerator would also go down proportionally (that's the whole purpose of the normalization, to get similar answers with many or few pixels of overlap). However since the moving image is expanded the space between overlapping pixels is larger than one pixel in the fixed image. This gives the algorithm an unwanted degree of freedom to work with: if a particular pixel infixedcan't be matched well then themovingimage can be positioned to skip over that pixel so that it doesn't get compared to amovingpixel and never enters into the mismatch calculation. With larger scale factors the algorithm gets more and more freedom to avoid unwanted pixel comparisons.This can be seen as a kind of aliasing, so one way that I could imagine addressing this particular issue is to lowpass filter the
fixedimage before comparing it to an expandedmovingimage. I'm not convinced this would solve all issues but it may help. Another idea is to address this by changing the default indices wherewarpgets evaluated so that themovingimage gets oversampled and we avoid skipping pixels infixed. I kind of prefer the second idea, but this seems to require thatwarpallow evaluations at non-integer indices which doesn't seem possible right now. Since neither of these seems extremely easy to implement I didn't include them in this PR.And by the way I tried to reduce the runtime of the tests, but they're still taking 2 minutes on my laptop. Not sure how long that would take on Travis...