ENH/WIP: added Apply VDM functionality to FieldMap SPM interface #2879
Conversation
…roc workflow. useful for combining 4d realign with fieldmap correction
Merge remote-tracking branch 'upstream/master'
Codecov Report
@@ Coverage Diff @@
## master #2879 +/- ##
=========================================
Coverage ? 67.48%
=========================================
Files ? 343
Lines ? 43628
Branches ? 5432
=========================================
Hits ? 29442
Misses ? 13483
Partials ? 703
Continue to review full report at Codecov.
|
|
Hi @fabioboh, this looks like a great contribution! Looking at it now, however, I wonder if it might not be better to split this into a separate |
|
Hi Chris,
thanks for your encouraging words, and sorry for my delay in answering, I
was on holidays (with very bad internet connection). I agree that there is
little shared logic between calculateVDM and applyVDM. I implemented the
nipype class as an interface to the FieldMap SPM toolbox, which implements
both the calculation of the VDM and its application to MRI images. I think
this is more intuitive for the average SPM user than having an ApplyVDM
class and another class called CalculateVDM. However, if you (or other
users!) have a strong opinion that this solution might be better, I think I
can implement it easily.
Have a nice day
Fabio
Il giorno gio 14 feb 2019 alle ore 17:12 Chris Markiewicz <
notifications@github.com> ha scritto:
… Hi @fabioboh <https://github.com/fabioboh>, this looks like a great
contribution! Looking at it now, however, I wonder if it might not be
better to split this into a separate ApplyVDM interface, as there is
little shared logic with the existing calculatevdm mode. Let me know what
you think.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2879 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ATEc7Q0H-ub-WTyITAzN1vVAeXTd38kmks5vNYrXgaJpZM4a0RT1>
.
--
Dr. Fabio Bernardoni
Prießnitzstr. 54
01099 Dresden
+49 176 9947 0149
|
|
Hi @effigies since I did not get any answer to my last message on this I wonder whether my changes have been integrated in the new nipype version. It is of interest for me because we are planning to move to the new version. I guess they were not merged in the version discussed in this thread. Am I right? Thanks Fabio |
|
Ah, sorry for not responding, this fell off my plate. Possibly I was waiting to see if anybody else had opinions. In any event, no, this has not been merged. Regarding the new interface versus one interface with two options, that's honestly up to contributors. There are several cases throughout nipype where the contributor preferred to create a new interface for each conceptual function of a given program, and others where they preferred to mimic the original interface in all its gory complexity. I won't push either option on you, though as you can probably tell, I find the former simpler. A somewhat complicating issue here is that, at this point, the codebase has moved on fairly substantially (for example, we've dropped Python 2 support and restyled the entirety of the code). You may want to do something like: And only add the relevant changes. |
|
Hi Chris,
thanks for these suggestions but I am a bit confused. Isn't it enough to
resolve the conflicts from the git hub webpage? Or if I have followed the
above steps locally, how do I integrate them? A new pull request?
Il giorno mar 21 gen 2020 alle ore 16:41 Chris Markiewicz <
notifications@github.com> ha scritto:
… Ah, sorry for not responding, this fell off my plate. Possibly I was
waiting to see if anybody else had opinions. In any event, no, this has not
been merged.
Regarding the new interface versus one interface with two options, that's
honestly up to contributors. There are several cases throughout nipype
where the contributor preferred to create a new interface for each
conceptual function of a given program, and others where they preferred to
mimic the original interface in all its gory complexity. I won't push
either option on you, though as you can probably tell, I find the former
simpler.
------------------------------
A somewhat complicating issue here is that, at this point, the codebase
has moved on fairly substantially (for example, we've dropped Python 2
support and restyled the entirety of the code). You may want to do
something like:
black nipype/interfaces/spm/preprocess.py
cp nipype/interfaces/spm/preprocess.py tmp.py
git fetch upstream
git reset --hard upstream/master
cp tmp.py nipype/interfaces/spm/preprocess.py
git add -p
And only add the relevant changes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2879?email_source=notifications&email_token=AEYRZ3OUCFHT4BPAZBXSHULQ64JS5A5CNFSM4GWRCT22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJQF3LI#issuecomment-576740781>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYRZ3KWPEW4P5VCWWMHFCTQ64JS5ANCNFSM4GWRCT2Q>
.
--
Dr. Fabio Bernardoni
Prießnitzstr. 54
01099 Dresden
+49 176 9947 0149
|
|
Hi Fabio. Once you've done the above, you will need to Sorry for the very slow response... |
|
I will try to solve this once for all now. I will recreate a merge request from the latest nipype version and resubmit it. |
First pull request ever to github project: I followed the contributing instructions but please let me know if anything was done incorrectly.
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