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

ENH: add gradunwarp base workflow for f/smriprep. #819

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link
Contributor

@bpinsard bpinsard commented Aug 2, 2023

Working on nipreps/fmriprep#1550.
Relies on an updated version of the gradunwarp https://github.com/bpinsard/gradunwarp/tree/fix/mem_leaks.

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Attention: Patch coverage is 48.48485% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 62.96%. Comparing base (a9f00d5) to head (2ec6bfc).

Current head 2ec6bfc differs from pull request most recent head 2c1507a

Please upload reports for the commit 2c1507a to get more accurate results.

Files Patch % Lines
niworkflows/workflows/gradunwarp.py 18.51% 22 Missing ⚠️
niworkflows/interfaces/gradunwarp.py 67.56% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #819      +/-   ##
==========================================
- Coverage   68.52%   62.96%   -5.57%     
==========================================
  Files          86       55      -31     
  Lines        8220     6316    -1904     
  Branches      949      914      -35     
==========================================
- Hits         5633     3977    -1656     
+ Misses       2580     2152     -428     
- Partials        7      187     +180     
Flag Coverage Δ
reportlettests ∅ <ø> (?)
unittests ∅ <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oesteban
Copy link
Member

oesteban commented Aug 7, 2023

I'm guessing if you want circle / gh to run the test, you'll need to install gradunwarp right?

Comment on lines 72 to 77
itk_warp_data = fsl_warp.get_fdata().reshape(fsl_warp.shape[:3]+(1,3))
itk_warp_data[...,(0,1)] *= -1
itk_warp = fsl_warp.__class__(itk_warp_data, fsl_warp.affine)
itk_warp.header.set_intent("vector")
out_fname = fname_presuffix(in_warp, suffix="_itk", newpath=os.getcwd())
itk_warp.to_filename(out_fname)
Copy link
Member

Choose a reason for hiding this comment

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

nitransforms should do all the manipulations for you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might have missed something, but from what I understood what needs to be provided to Ants ApplyTransform are ITK warpfields. In nitransforms I cannot see any function to output non-linear warp in ITK format from nitransforms object https://nitransforms.readthedocs.io/en/latest/_api/io.html#nitransforms.io.itk.ITKDisplacementsField, it seems to only deal with inputs. Thus, my attempt to invert that input step.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like you are totally right (and I did the same here a while ago - https://github.com/nipreps/sdcflows/blob/375dd7a024abcd5d4d2b282241a037d76f9719e6/sdcflows/transform.py#L286-L292)

Would you like to become a proud contributor to nitransforms? :D

If not (or either way, actually), may I ask you to file an issue requesting this to be implemented there?

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.

Mostly comments on the nipype interface.

niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/interfaces/gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/workflows/tests/test_gradunwarp.py Outdated Show resolved Hide resolved
niworkflows/workflows/tests/test_gradunwarp.py Outdated Show resolved Hide resolved
@bpinsard bpinsard force-pushed the enh/gradunwarp branch 2 times, most recently from 559c8a1 to a4d7e3b Compare May 15, 2024 18:18
@bpinsard bpinsard marked this pull request as ready for review May 16, 2024 19:19
@bpinsard
Copy link
Contributor Author

I don't understand why I am getting errors on the T1w file when testing the gradunwarp pipeline. How does that ds0000030 differs from the openeuro one?

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