ENH: Upgrade SDCFlows to new API (1.0.0rc1) #1886
Conversation
|
Thank your for raising your pull request. Some of the fMRIPRep maintainers will review your changes as soon as time permits. ## PR Review
*Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide*
- [ ] As the reviewer I confirm that there are no conflicts of interest for me to review this work.
Please check what applies in the following aspects of the PR:
#### Code documentation
- [ ] New functions and modules are documented following the guidelines.
- [ ] Existing code required amendments in documentation and has been updated.
- [ ] Sufficient inline comments ensure readability by other contributors in the long term.
#### Documentation site
- [ ] The modifications to *fMRIPrep* in this PR **do not require extra documentation**. This is a common situation when a bug fix that does not change the code base substantially is submitted.
- [ ] This PR requires documentation. The corresponding documentation will be submitted as an additional PR in the future.
- [ ] This PR requires extra documentation, and will be included within this PR. Please check the boxes that apply best:
- [ ] An existing feature is substantially modified, changing the interface and/or logic of a module.
- [ ] A new feature is being added, therefore full documentation of it will be included
- [ ] This PR addresses an error in existing documentation.
- [ ] Yes, all expected sections of documentation were generated by the CI build, and look as expected
#### Tests
- [ ] The new functionalities in this PR are covered by the existing tests
- [ ] New test batteries have been included with this PR
#### Data
- [ ] This PR does not require any additional data to be uploaded to OSF.
- [ ] This PR requires additional data for tests.
#### Dependencies: smriprep
- [ ] This PR does not require any update on `smriprep`; or
- [ ] `smriprep` has correctly been pinned.
#### Dependencies: niworkflows
- [ ] This PR does not require any update on `niworkflows`; or
- [ ] `niworkflows` has correctly been pinned.
#### Dependencies: sdcflows
- [ ] This PR does not require any update on `sdcflows`; or
- [ ] `sdcflows` has correctly been pinned.
#### Dependencies: Nipype
- [ ] This PR does not require any update on `nipype`; or
- [ ] `nipype` has correctly been pinned.
#### Dependencies: other
- [ ] This PR does not require any update on other dependencies; or
- [ ] other dependencies have correctly been pinned.
#### Reports generated within CI tests
- [ ] Yes, I have checked the reports of ds005 and they are not modified or the changes are as expected
- [ ] Yes, I have checked the reports of ds054 and they are not modified or the changes are as expected
- [ ] Yes, I have checked the reports of ds010 and they are not modified or the changes are as expected
### Review Comments (other than those left inline with the code)
|
oesteban
force-pushed
the
rf/new-sdcflows-api
branch
2 times, most recently
from
5f01a9a
to
b21a93e
Compare
|
Please note the new condensed view for phase1/2-phasediff-fieldmap kind of fieldmaps (ds054). This PR merges together the magnitude segmentation view, the magnitude registration view and the fieldmap registration view (hopefully you like the new one better). I think we still need to add a colorbar in Hz to the fieldmap overlay, and make it symmetric bc right now it is a bit confusing. But overall I believe it is much better. |
|
Another focus point (esp. for @effigies) is nipreps/sdcflows#63, which is necessary for this PR to work. |
|
Last comment - this PR should be then followed by addressing #1892, ensuring we add MNI152NLin2009cAsym to the standard spaces list if SyN is to run. |
|
Should also fix #1665. |
|
The actual fieldmaps are fine, but the fieldmap report figures have these values spilling over the y-axis.
Actually, probably an issue on sdcflows. |
|
That extrapolation is added by FUGUE. I agree we should open an issue in SDCFlows about it, to double-check that the extrapolation is done once the fieldmap has been mapped into the target EPI. The alternative to this, once again, is implementing nipreps/sdcflows#14 so that extrapolation is less of a blanket across the PE-axis solution. |
|
Sorry, missed this in a flood of notifications... I'll review now. |
…0][skip tests] We need to start looking into intersphinx and how we are going to coordinate fMRIPrep's documentation and helper packages' documentation.
oesteban
force-pushed
the
rf/new-sdcflows-api
branch
from
434c947
to
9f14aba
Compare
|
Okay, updated the pin to a released version since the patch is already in SDCFlows/master. That was breaking builds. |
Follows up on nipreps#1886 (comment)
Closes #1884.
Changes proposed in this pull request
Documentation that should be reviewed