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

TST+FIX: LTA conversions #36

Merged
merged 12 commits into from
Oct 29, 2019
Merged

TST+FIX: LTA conversions #36

merged 12 commits into from
Oct 29, 2019

Conversation

mgxd
Copy link
Member

@mgxd mgxd commented Oct 28, 2019

No description provided.

@oesteban
Copy link
Collaborator

I think it won't change your tests, but I think this line has source and destination swapped:
https://github.com/poldracklab/nitransforms/blob/45a0a2fe4268714b0f7237bab65cb084d806fa68/nitransforms/io/lta.py#L243

That line would work okay if xform['m_L'] were the ras2ras matrix and you wanted to calculate the vox2vox matrix from it. However, we want to do the opposite:

  • Let R be a matrix representing a ras2ras transform and V the matrix representing the equivalent vox2vox transform.
  • Let Asrc and Adst be the affines of source and destination, respectively.

Then, R = inv(Adst) . V . Asrc.

So, if we want to extract V from R, we need to multiply the inverses of each side:

            M = dst.as_affine().dot(xform['m_L'].dot(np.linalg.inv(src.as_affine())))

@mgxd
Copy link
Member Author

mgxd commented Oct 28, 2019

yeah, i was just looking at how LTA convert implements and you got it
https://github.com/freesurfer/freesurfer/blob/933dc7794c15243e7f0370e912c6e877b87eda66/utils/transform.cpp#L3707

Even changing the algorithm, I'm still not sure why it is not working in my example...

@oesteban
Copy link
Collaborator

Can you update the line with my suggestion?

@mgxd
Copy link
Member Author

mgxd commented Oct 29, 2019

Okay - I decided to use the affine-RAS.fs.lta as the baseline (RAS2RAS), and convert it to VOX2VOX using lta_convert. There is still a slight discrepancy, albeit much smaller than before.

In [3]: v2v_m                                                                                                                                                                                                                               
Out[3]: 
array([[ 9.9999899e-01, -9.9999935e-04,  9.9999981e-04,  3.9999959e+00],
       [ 1.4049364e-03,  6.2160885e-01, -7.8332651e-01,  1.9999948e+00],
       [ 1.6171722e-04,  7.8332722e-01,  6.2160969e-01, -9.9998468e-01],
       [ 0.0000000e+00,  0.0000000e+00,  0.0000000e+00,  1.0000000e+00]],
      dtype=float32)

In [4]: r2r_m                                                                                                                                                                                                                               
Out[4]: 
array([[ 9.9999899e-01, -9.9999935e-04,  9.9999981e-04,  4.0000000e+00],
       [ 1.4049363e-03,  6.2160885e-01, -7.8332651e-01,  2.0000000e+00],
       [ 1.6171722e-04,  7.8332716e-01,  6.2160963e-01, -1.0000000e+00],
       [ 0.0000000e+00,  0.0000000e+00,  0.0000000e+00,  1.0000000e+00]],
      dtype=float32)

@mgxd mgxd marked this pull request as ready for review October 29, 2019 15:50
@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #36 into master will increase coverage by 2.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #36      +/-   ##
==========================================
+ Coverage   91.92%   94.08%   +2.15%     
==========================================
  Files           8        8              
  Lines         570      575       +5     
  Branches       75       77       +2     
==========================================
+ Hits          524      541      +17     
+ Misses         30       18      -12     
  Partials       16       16
Flag Coverage Δ
#unittests 94.08% <100%> (+2.15%) ⬆️
Impacted Files Coverage Δ
nitransforms/linear.py 78.33% <ø> (ø) ⬆️
nitransforms/io/lta.py 98.57% <100%> (+8.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 455f337...6733cba. Read the comment docs.

Copy link
Collaborator

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Left a few comments, but this is looking great. Thanks a lot!

nitransforms/io/lta.py Outdated Show resolved Hide resolved
nitransforms/io/lta.py Outdated Show resolved Hide resolved
nitransforms/io/lta.py Outdated Show resolved Hide resolved
nitransforms/io/lta.py Outdated Show resolved Hide resolved
nitransforms/linear.py Outdated Show resolved Hide resolved
nitransforms/io/lta.py Outdated Show resolved Hide resolved
nitransforms/io/lta.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Only one minor nitpick

nitransforms/io/lta.py Show resolved Hide resolved
@oesteban oesteban merged commit 71277ae into nipy:master Oct 29, 2019
@mgxd mgxd deleted the tst/lta branch October 31, 2019 19:53
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