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: added Apply VDM functionality to FieldMap SPM interface #3394

Merged
merged 13 commits into from
Oct 21, 2021

Conversation

fabioboh
Copy link
Contributor

@fabioboh fabioboh commented Oct 19, 2021

This pull request substitutes an old one:

ENH/WIP: added Apply VDM functionality to FieldMap SPM interface #2879

I only adapted what was done there to the present nipype version.

This allows to apply a VDM directly to functional images to correct for inhomogeneities of the magnetic field. At the moment this could be performed by the realign&unwarp spm interface, which also corrects for artefacts derived by interactions between subject movement and inhomogeneities of the magnetic field. However, if one wishes to perform slice timing and realign correction in one step, using the SpaceTimeRealigner nipy module, this requires the field map correction to be executed independently from the realignment.

This was also a TODO item for issue:
[ENH/WIP] Add SPM Fieldmap Tool wrapper #1905

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@fabioboh fabioboh changed the title ENH/WIP: added Apply VDM functionality to FieldMap SPM interface ENH: added Apply VDM functionality to FieldMap SPM interface Oct 19, 2021
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #3394 (4575810) into master (bb3c312) will decrease coverage by 0.01%.
The diff coverage is 42.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3394      +/-   ##
==========================================
- Coverage   65.21%   65.20%   -0.02%     
==========================================
  Files         307      307              
  Lines       40413    40457      +44     
  Branches     5344     5350       +6     
==========================================
+ Hits        26356    26379      +23     
- Misses      12982    13003      +21     
  Partials     1075     1075              
Flag Coverage Δ
unittests 64.92% <42.55%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
nipype/interfaces/spm/__init__.py 100.00% <ø> (ø)
nipype/sphinxext/plot_workflow.py 14.95% <ø> (ø)
nipype/interfaces/spm/preprocess.py 50.23% <42.55%> (-0.21%) ⬇️
nipype/info.py 92.18% <0.00%> (+4.68%) ⬆️

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 bb3c312...4575810. Read the comment docs.

@fabioboh
Copy link
Contributor Author

Since this is an old, tiny pull request and it already fell into oblivion once, since it seems to me that all tests are passed, I would be grateful if it could be considered quickly.

Makefile Outdated Show resolved Hide resolved
@effigies
Copy link
Member

It looks like many unrelated files got changed. You may need to update black in your environment.

@fabioboh
Copy link
Contributor Author

fabioboh commented Oct 20, 2021 via email

@fabioboh
Copy link
Contributor Author

So after reverting some changes and rerunning the tests I think my commit is ready to be accepted.

@effigies
Copy link
Member

Had a quick look, and I think we need to make this a new interface, not an update to the existing one. This changes its behavior too much, disabling mandatory inputs and making new inputs mandatory. You could do some fancy things with making inputs conditionally mandatory, but it would probably be simpler to just make it an ApplyVDM interface.

@fabioboh
Copy link
Contributor Author

I implemented the nipype class as an interface to the FieldMap SPM toolbox, which performs
both the calculation of the VDM and its application to MRI images because I thought this would be more intuitive to the end user. However, what would you suggest precisely:
1- have two interfaces, CalculateVDM and ApplyVDM. (but people around the world will have errors when updating to the new version)

2- have two interfaces, FieldMap and ApplyVDM. Leave FieldMap as it was (except removing the jobtype argument) and add ApplyVDM

?

@fabioboh
Copy link
Contributor Author

This commit implements solution 2 above.

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.

Overall looks good. Thanks for pushing ahead with option 2, that would have been my suggestion.

nipype/interfaces/spm/preprocess.py Show resolved Hide resolved
nipype/interfaces/spm/preprocess.py Outdated Show resolved Hide resolved
nipype/interfaces/spm/preprocess.py Outdated Show resolved Hide resolved
nipype/interfaces/spm/preprocess.py Outdated Show resolved Hide resolved
nipype/interfaces/spm/preprocess.py Outdated Show resolved Hide resolved
* added applyVDM interface to __init__.py file
@fabioboh
Copy link
Contributor Author

I have implemented the suggestions above and edited the init file, so from my side all is ready for acceptance.

Fabio Bernardoni and others added 2 commits October 21, 2021 13:27
@effigies
Copy link
Member

Looks good! Thanks for your patience.

@effigies effigies merged commit eab4b2a into nipy:master Oct 21, 2021
effigies added a commit that referenced this pull request Oct 25, 2021
[FIX] small edits to my previous pull request (PR #3394) due to bugs revealed when running within my nipype workflow
@effigies effigies added this to the 1.7.1 milestone Apr 3, 2022
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.

2 participants