Skip to content

ENH: Speed up tractogram.apply_affine #1000

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
merged 3 commits into from
Feb 10, 2022
Merged

Conversation

MarcCote
Copy link
Contributor

This PR adds the inplace option to affines.apply_affine(...) which Tractogram.apply_affine can leverage for some speed and memory gains.

The speedup is actually two-fold:

  1. Allowing affines.apply_affine to perform the transformation in-place reduces memory peak usage (esp. when dealing with 10 million streamlines).
  2. If the ArraySequence representing the tractogram's streamlines is not a sliced view, the affine transformation can be applied directly on the underlying numpy array that contains the data. Doing that avoids having to transform each streamline one by one as it is currently done.

Also, this PR partially addresses issue #943 (see my comment over there for more details).

@codecov
Copy link

codecov bot commented Feb 25, 2021

Codecov Report

Merging #1000 (8c20100) into master (4703f4d) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 8c20100 differs from pull request most recent head cb037d0. Consider uploading reports for the commit cb037d0 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1000      +/-   ##
==========================================
- Coverage   92.30%   92.23%   -0.08%     
==========================================
  Files         100      100              
  Lines       12223    12176      -47     
  Branches     2194     2131      -63     
==========================================
- Hits        11282    11230      -52     
- Misses        617      618       +1     
- Partials      324      328       +4     
Impacted Files Coverage Δ
nibabel/affines.py 100.00% <100.00%> (ø)
nibabel/streamlines/array_sequence.py 99.30% <100.00%> (+0.66%) ⬆️
nibabel/streamlines/tractogram.py 99.68% <100.00%> (+<0.01%) ⬆️
nibabel/filebasedimages.py 87.23% <0.00%> (-2.13%) ⬇️
nibabel/loadsave.py 92.59% <0.00%> (-1.26%) ⬇️
nibabel/nicom/csareader.py 87.40% <0.00%> (-0.75%) ⬇️
nibabel/volumeutils.py 93.46% <0.00%> (-0.71%) ⬇️
nibabel/openers.py 95.09% <0.00%> (-0.63%) ⬇️
nibabel/nifti1.py 92.10% <0.00%> (-0.47%) ⬇️
nibabel/dft.py 80.98% <0.00%> (-0.31%) ⬇️
... and 12 more

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 4703f4d...cb037d0. Read the comment docs.

@MarcCote
Copy link
Contributor Author

@effigies @arokem I forgot about this PR until now. Some TRK's header issue was brought up in this discussion: https://neurostars.org/t/trk-files-in-dipy-header-file-incorrect/20452/6. I'm not sure if the issue is related to Dipy/NiBabel or some other library.

@arokem
Copy link
Member

arokem commented Oct 27, 2021

This looks good to me!

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Sorry for the extremely slow review. This LGTM.

@effigies effigies merged commit b8445fc into nipy:master Feb 10, 2022
@MarcCote MarcCote deleted the fix_943 branch February 23, 2022 02:26
@MarcCote
Copy link
Contributor Author

@effigies no worries. Happy it's in before 4.0.0 :).

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