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] Refactor BOLD workflows #805

Merged
merged 2 commits into from Oct 31, 2017
Merged

Conversation

oesteban
Copy link
Member

Create a bold module that makes it more manageable contributing to FMRIPREP.

Generation of API for workflows has been revised (refs. #615).

Create a `bold` module that makes more manageable contributing
to FMRIPREP.
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.

Looks reasonable. didn't check, but I assume the contents of the workflows are effectively unchanged.


.. autofunction:: init_func_preproc_wf
.. autofunction:: init_func_reports_wf
.. autofunction:: init_func_derivatives_wf
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to include reports and derivatives in the API? I think I skipped them. But I suppose no harm, either.

Copy link
Member Author

Choose a reason for hiding this comment

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

It generates two entries with one-liner description. May even help to newcomers.

# T | * | F | Fieldmaps
# F | * | T | SyN
# F | T | F | SyN
# F | F | F | HMC only
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to do this, should we make the choice of fieldmap unwarping and SyN-based unwarping part of the fieldmaps workflows? Though, that may make more sense as a separate PR, as this one is already pretty big. Either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - the SyN-based workflow should be attached the same way we are currently doing with the fieldmap decision maker in workflows.fieldmap.base if we want to be consistent. But better in a different PR where that's the only target.

@oesteban
Copy link
Member Author

Seems like everything is in place. Gong ahead.

@oesteban oesteban merged commit e170de1 into nipreps:master Oct 31, 2017
@oesteban oesteban deleted the enh/refactor-bold branch October 31, 2017 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants