Skip to content
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

mrtransform: Bug fix for warp field regridding #1134

Merged
merged 1 commit into from
Sep 22, 2017

Conversation

Lestropie
Copy link
Member

If non-linear warp field and template image are not defined on the same voxel grid, the warp field must be resampled onto the template grid before warping can take place. This needs to be done using an identity transform rather than an uninitialised transform.

Copy link
Member

@rtabbara rtabbara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me. I think (hope) that the default-constructor initialised the underlying matrix to identity, otherwise it's surprising that this wasn't picked up sooner.

Regardless, this is cleaner anyway so it's all good.

@Lestropie
Copy link
Member Author

transform_type uninitialised_transform;
std::cerr << uninitialised_transform.matrix() << "\n";
5.72622e-321  6.9428e-310 4.67708e-310 2.26486e-306
 6.9428e-310 3.95253e-323 4.67708e-310 1.18576e-322
7.90505e-323 6.95309e-310 4.67688e-310 3.18642e-318

:-/ Maybe the behaviour changed with Eigen 3.3?

@rtabbara
Copy link
Member

@Lestropie Whelp, I get something similar on Eigen 3.2. Think I got it confused with https://github.com/MRtrix3/mrtrix3/blob/master/cmd/mrregister.cpp#L805 which does identity-initialise the underlying matrices.

Anyway, great catch, although I still don't get how previously multiplying a junk matrix when reslicing would have gone unnoticed 😬

@Lestropie Lestropie merged commit bc31599 into master Sep 22, 2017
@Lestropie Lestropie mentioned this pull request Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants