Skip to content

Comments

Fixed order of composed transformations#79

Merged
timholy merged 2 commits intoHolyLab:masterfrom
ChantalJuntao:JC/transformation_change
Jul 4, 2018
Merged

Fixed order of composed transformations#79
timholy merged 2 commits intoHolyLab:masterfrom
ChantalJuntao:JC/transformation_change

Conversation

@ChantalJuntao
Copy link
Contributor

Fixes the order of transformations in the coarse step, which creates errors with large translations

@timholy
Copy link
Member

timholy commented Jul 3, 2018

The test failure seems unrelated to these changes.

tfmcoarse0 = rot(position(box_coarse, x0coarse), moving)
best_shft, mm = best_shift(fixed, moving, mxshift, thresh; normalization=:intensity, initial_tfm = tfmcoarse0)
tfmcoarse = Translation(best_shft) ∘ tfmcoarse0
tfmcoarse = tfmcoarse0 ∘ Translation(best_shft)
Copy link
Contributor

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 mm value 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. Since tfmcoarse0 is 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.

Copy link
Member

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 m is the moving image and t1 is the transform that you rotate it with, you produce a new image m1. Mathematically, we can think of this as defining a function (if you think about images as being continuous rather than discrete) as

m1(x) = m(t1(x))

because in warp the transformation operates on the coordinates at which you evaluate the image. Now we take m1 as the new moving image and calculate a new transformation t2 (which happens to be a translation). At the end we get m2 which is defined

m2(x) = m1(t2(x)) = m(t1(t2(x)))

The 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.

Copy link
Contributor

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!

@Cody-G
Copy link
Contributor

Cody-G commented Jul 3, 2018

As I say in the code comment I'm not sure why this works, but I still didn't explain the fact that the test passes (and I assume fails without the change in order). The test case is not as easy as it might seem because to create the warped image you did a rotation followed by a translation. The algorithm must find its answer as a rotation followed by a translation in a 2-step process (even though the most intuitive way to invert the transform would be to do a translation followed by a rotation). That means that the optimal rotation --> translation to invert the original transform may involve much larger translation/rotation values. In that test case in particular I would expect that the translation step would need to be quite large to get back to the moving image.

@Cody-G
Copy link
Contributor

Cody-G commented Jul 4, 2018

After that revelation this looks good to merge :-)

@Cody-G
Copy link
Contributor

Cody-G commented Jul 4, 2018

After we merge this I will rebase https://github.com/HolyLab/BlockRegistration/commits/cjg/quaddirect_tweaks on it and check for further failures related to the ordering issue. The main additions in that branch are QuadDIRECT-based methods for optimizing translations and full affine transforms.

@timholy timholy merged commit caed5e8 into HolyLab:master Jul 4, 2018
@Cody-G Cody-G mentioned this pull request Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants