-
Notifications
You must be signed in to change notification settings - Fork 16
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
FIX: Increase FSL serialization precision #144
Conversation
@@ -46,14 +46,14 @@ def from_ras(cls, ras, moving=None, reference=None): | |||
|
|||
# Adjust for reference image offset and orientation | |||
refswp, refspc = _fsl_aff_adapt(reference) | |||
pre = reference.affine.dot(inv(refspc).dot(inv(refswp))) | |||
pre = reference.affine @ inv(refswp @ refspc) |
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 think it makes sense to think of the matrices as aff
and swp @ spc
. So you are always either aff @ inv(swp @ spc)
or swp @ spc @ inv(aff)
.
|
||
# Adjust for moving image offset and orientation | ||
movswp, movspc = _fsl_aff_adapt(moving) | ||
post = inv(movswp).dot(movspc.dot(inv(moving.affine))) | ||
post = movswp @ movspc @ inv(moving.affine) |
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.
That catches that here you had movswp
inverted and movspc
not.
2ca2724
to
77d93fd
Compare
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.
Thanks for this. I've updated the test files so that the new precision is honored.
@@ -24,4 +24,4 @@ def assert_affines_by_filename(affine1, affine2): | |||
else: | |||
xfm1 = np.loadtxt(str(affine1)) | |||
xfm2 = np.loadtxt(str(affine2)) | |||
np.testing.assert_almost_equal(xfm1, xfm2) | |||
np.allclose(xfm1, xfm2, atol=1e-04) |
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.
We just lost an assertion.
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.
ugh good catch
Was reviewing when you merged. :-)