ENH: Allow bold_reference_wf to accept multiple EPIs/SBRefs #408
Conversation
Codecov Report
@@ Coverage Diff @@
## master #408 +/- ##
=========================================
- Coverage 59.05% 53.46% -5.6%
=========================================
Files 45 59 +14
Lines 5002 5630 +628
Branches 681 681
=========================================
+ Hits 2954 3010 +56
- Misses 2002 2565 +563
- Partials 46 55 +9
Continue to review full report at Codecov.
|
effigies
left a comment
There was a problem hiding this comment.
Looks pretty good overall. Some suggestions. Is there to be an fMRIPrep patch that depends on this? It looks like with the switch to MapNode, we're going to have to have some simultaneous changes downstream.
@oesteban Do we use init_bold_reference_wf in MRIQC or anywhere else?
|
@effigies thanks! I've made the requested changes. I do plan to open a patch PR to fMRIPrep to match this one. The tests seem to be failing due to a random connection error. |
|
Before you change any code in fMRIPrep, can you just update the niworkflows requirement, to see if that breaks tests? If we can maintain backwards compatibility, I think that would be ideal. If we don't currently have it, I think we can fix it up with: workflow.connect([
(inputnode, enhance_and_skullstrip_bold_wf, [('bold_mask', 'inputnode.pre_mask')]),
- (inputnode, validate, [('bold_file', 'in_file')]),
+ (inputnode, validate, [(('bold_file', ensure_list), 'in_file')]),
(inputnode, gen_ref, [('sbref_file', 'sbref_file')]),
(inputnode, calc_dummy_scans, [('dummy_scans', 'dummy_scans')]), |
|
Hello @tsalo, Thank you for updating! Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, Comment last updated at 2020-06-07 02:12:47 UTC |
|
Don't worry about the pep8 speaks just yet... I need to configure it. |
|
Okay, should be configured. Can you merge |
|
Hmm. I don't understand why it's not getting the 99-character limit correct. I'll look into it. |
|
Got it. It was looking for config in |
|
Will do. Regarding fMRIPrep compatibility, what is the best way for me to check? I can run fMRIPrep (plus my niworkflows fork) locally in a Docker image, but is it enough to run with |
|
Oh sorry, I made a merge commit instead. Sorry about that. I assume that this can be squashed and merged though? To remove those commits? |
|
Eh, either way. But I don't think you're likely to run into any conflicts with: git fetch poldracklab
git rebase poldracklab/masterIf it doesn't rebase cleanly, just |
|
Cool, thanks. For fMRIPrep, I just mean have a commit where you update the niworkflows dependency to |
|
Okay. Hopefully last rebase needed... |
oesteban
left a comment
There was a problem hiding this comment.
I'm worried these changes are very implicit, before going ahead we should set up several tests. Happy to help with that.
|
Just wanted to ping this PR. The last time I checked, there was still some disagreement about the behavior of |
|
Hi, this is waiting for me to test it locally. Thank for your patience.
…On Thu, Oct 24, 2019, 05:15 Taylor Salo ***@***.***> wrote:
Just wanted to ping this PR. The last time I checked, there was still some
disagreement about the behavior of InputMultiObject. Should I treat
single-item InputMultiObjects as strings or lists?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#408>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRRIOKHLE2JQIFTEDNLQQGGWPANCNFSM4I4IRMJQ>
.
|
|
No pressure! I just wanted to make sure it wasn't waiting on me for anything. |
|
Complexity increasing per file
==============================
- niworkflows/interfaces/registration.py 4
- niworkflows/func/util.py 2
See the complete overview on Codacy |
Codecov Report
@@ Coverage Diff @@
## master #408 +/- ##
==========================================
- Coverage 64.50% 64.16% -0.34%
==========================================
Files 43 44 +1
Lines 5341 5372 +31
Branches 778 784 +6
==========================================
+ Hits 3445 3447 +2
- Misses 1745 1774 +29
Partials 151 151
Continue to review full report at Codecov.
|
|
I'm going to make this a draft so that it is isn't accidentally merged without updating the regression data (which I'll do right before merging). |
This is necessary as now the masks regressions will only be run on the first echo of ME datasets. This id bump will only be applied after merging into master, as the regression tests are run only on master or branches with the prefix ``masks?/``. Reference: #1.
validates incoming sbrefs as suggested in nipreps/fmriprep#1803 (comment)
Because that function utilizes ``is_container``, which is imported at global context, nipype could not evaluate ``ensure_list`` calls inlined inside ``connect``. This would end up failing with: ``` fMRIPrep failed: name 'is_container' is not defined ``` A new module for nipype connections is created to provide this kind of commodity functions for all NiPreps.
|
Thumbs up? |
|
Okay, I just uploaded the new masks and started https://app.circleci.com/pipelines/github/oesteban/niworkflows/115/workflows/3236dc2a-0115-4f3e-88d5-ab31cdd332f4 Once that build is passing, I'll merge this PR. |
First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (#554) * FIX: Add dots to extensions in PyBIDS' config file (#548) * FIX: Segmentation plots aligned with cardinal axes (#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (#485) * ENH: Add a pure-Python interface to resample to specific resolutions (#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (#550) * MAINT: Update to Python +3.6 (#541)
1.3.0rc3 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS version of fMRIPrep. PyBIDS has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into Nipype, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)
1.3.0 First release in the 1.3.x series. This release includes enhancements and bug-fixes towards the release of the first LTS (*long-term support*) version of *fMRIPrep*. *PyBIDS* has been revised to use more recent versions, a series of ANTs' interfaces have been deemed ready to upstream into *Nipype*, and several improvements regarding multi-echo EPI are included. With thanks to Basile Pinsard for contributions. * FIX: Patch ``ApplyTransforms`` spec to permit identity in a chain (nipreps#554) * FIX: Add dots to extensions in PyBIDS' config file (nipreps#548) * FIX: Segmentation plots aligned with cardinal axes (nipreps#544) * FIX: Skip T1w file existence check if ``anat_derivatives`` are provided (nipreps#545) * FIX: Avoid diverting CIFTI dtype from original BOLD (nipreps#532) * ENH: Add ``smooth`` input to ``RegridToZooms`` (nipreps#549) * ENH: Enable ``DerivativesDataSink`` to take multiple source files to derive entities (nipreps#547) * ENH: Allow ``bold_reference_wf`` to accept multiple EPIs/SBRefs (nipreps#408) * ENH: Numerical stability of EPI brain-masks using probabilistic prior (nipreps#485) * ENH: Add a pure-Python interface to resample to specific resolutions (nipreps#511) * MAINT: Upstream all bug-fixes in the 1.2.9 release * MAINT: Finalize upstreaming of ANTs' interfaces to Nipype (nipreps#550) * MAINT: Update to Python +3.6 (nipreps#541)
Closes #399.
This PR allows
bold_reference_wfto accept multiple BOLD and/or SBRef files, under the assumption that multiple files reflect multiple echoes from the same run. It currently just selects the first image from each for use, but could be extended to use information from multiple echoes for reference image generation.The workflow still returns just one BOLD file, since a number of workflows take the BOLD file stored in the output node as their inputs, and they are not currently set up to work with multi-echo data. To that end I also added a function to
utils.miscto select the first item in a list.These changes shouldn't impact behavior at all, but there will need to be a corresponding PR in fMRIPrep in order to feed multi-echo data and SBRefs into the reference image workflow, when available.