-
Notifications
You must be signed in to change notification settings - Fork 1
Fixed order of composed transformations #79
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
Merged
timholy
merged 2 commits into
HolyLab:master
from
ChantalJuntao:JC/transformation_change
Jul 4, 2018
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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.
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.
After thinking on it I'm less certain that the new ordering is correct. The
mmvalue that we're using to choose a transform was computed by first rotating the image here and then finding the best translation of the rotated image here. Sincetfmcoarse0is the rotation then I think it should be applied first, i.e.Translation(best_shft) ∘ tfmcoarse0. Or am I missing something? Note that we redundantly compute the best shift again here (the redundancy is bad design on my part) but in both cases the rotation is applied first.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 agree this is confusing; and yes, the test fails (badly) without this. Here's how I now think about it: if
mis the moving image andt1is the transform that you rotate it with, you produce a new imagem1. Mathematically, we can think of this as defining a function (if you think about images as being continuous rather than discrete) asbecause in
warpthe transformation operates on the coordinates at which you evaluate the image. Now we takem1as the new moving image and calculate a new transformationt2(which happens to be a translation). At the end we getm2which is definedThe key thing to note is that we have
t1(t2(x)) = (t1 ∘ t2)(x). So it's the opposite of what I would have initially expected from the order in which these transformations were defined, which is probably why I didn't catch this when I reviewed #75.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.
Oh wow!! I'm surprised that I made it this long without reckoning with this misunderstanding... I think I inadvertently put a lot of effort into working around it. Thanks for that explanation, it's not intuitive but it makes perfect sense now!