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: Connect fieldmap estimation to preprocessing pipeline of individual DWI runs #140

Merged
merged 9 commits into from
Dec 11, 2020

Conversation

josephmje
Copy link
Collaborator

No description provided.

dmriprep/workflows/dwi/base.py Outdated Show resolved Hide resolved
dmriprep/workflows/dwi/base.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Dec 10, 2020

Hello @josephmje, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-12-11 14:50:57 UTC

Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

Looking good - tiny sill nit pick

dmriprep/workflows/dwi/base.py Outdated Show resolved Hide resolved
@oesteban oesteban changed the title WIP: Apply estimated fieldmap to dwi ENH: Connect fieldmap estimation to preprocessing pipeline of individual DWI runs Dec 11, 2020
@oesteban oesteban marked this pull request as ready for review December 11, 2020 09:13
oesteban added a commit to oesteban/sdcflows that referenced this pull request Dec 11, 2020
Allows for an interface similar to that of *sMRIPrep*, which enables using
``KeySelect`` nodes to find the appropriate connections downstream.

Required-by: nipreps/dmriprep#140.
Resolves: nipreps#147.
- Connections to the particular ``dwi_file`` are done within the
  particular run's preproc workflow, using ``KeySelect`` to demux
  the available fieldmap options.
  This relies on implementing nipreps/sdcflows#147 and
  nipreps/sdcflows#148.
- Minimizes the overhead in ``dmriprep/workflows/base.py``
- Run black
@josephmje
Copy link
Collaborator Author

@oesteban Thanks for the refactor to the base workflow. Reading it now and it looks a lot more intuitive.

oesteban added a commit to oesteban/sdcflows that referenced this pull request Dec 11, 2020
…EPI target

This code adds a ``get_identifier()`` interface that allows the user find the
a list of ``B0FieldIdentifier``(s) that are associated with a given file.

ATM, it pulls the information from the ``IntendedFor`` metadata of fieldmaps,
but in the future, it should first read the ``B0FieldSource``.

Originated-by: nipreps/dmriprep#140.
Resolves: nipreps#148.
@oesteban
Copy link
Member

@josephmje can you make sure you have pushed anything you were working on and hold off of pushing while I finish the SDCFlows updates? I'll update this branch when SDCFlows is ready.

@josephmje
Copy link
Collaborator Author

@josephmje can you make sure you have pushed anything you were working on and hold off of pushing while I finish the SDCFlows updates? I'll update this branch when SDCFlows is ready.

Yup everything I've been working on has been pushed.

@oesteban
Copy link
Member

Okay, if tests pass then I think we can go ahead and merge.

It still won't do anything with the fieldmaps because our test dataset with fieldmaps (ds001771) does not have IntendedFor and therefore, the registration and unwarping is disabled at https://github.com/nipreps/dmriprep/pull/140/files#diff-3f08760032797b48bfababab775f0a5f1852b0c33a7a0915122bc68d1bd52133R90

But I think that's a different issue (probably pertaining to SDCFlows, and namely the problem of what to do when no IntendedFor are given for a potential EPI target). And, as currently implemented, the behavior is similar to fMRIPrep's (which does nothing if IntendedFor is not set).

@josephmje
Copy link
Collaborator Author

Should we update the docstring for these workflows so that the fieldmap workflows get visualized? Currently, it refers to ds000206, which doesn't have any fieldmaps.

@oesteban
Copy link
Member

Should we update the docstring for these workflows so that the fieldmap workflows get visualized? Currently, it refers to ds000206, which doesn't have any fieldmaps.

Good luck with that :p

It's going to be a rabbit hole of massive dimensions. Plus, SDCFlows has ~94% coverage. Let's trust that.

@josephmje josephmje merged commit 44fec84 into nipreps:master Dec 11, 2020
@josephmje josephmje deleted the enh/apply_topup branch December 17, 2020 15:51
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