ENH: Remove t2s-coreg #2109

tsalo · 2020-04-29T00:59:15Z

Resolves #1786. This builds off of #1800, but removes the T2*-based coregistration option completely, based on the most recent comments in #1786 and discussion with @emdupre.

Changes proposed in this pull request

Remove --t2s-coreg option from CLI
Remove t2s_coreg-related steps from workflows.
Remove mention of --t2s-coreg option from docs/workflows.rst.

Documentation that should be reviewed

In docs/workflows.rst, I remove a sentence about --t2s-coreg.

auto-comment · 2020-04-29T00:59:20Z

Thank your for raising your pull request.

Some of the fMRIPRep maintainers will review your changes as soon as time permits.
I'm attaching below a Review Checklist for the reviewers, to check off during the
review.

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

oesteban

Haven't checked locally, but this looks pretty awesome. I'll try to review and get this merged tomorrow.

Thanks much for this!

effigies · 2020-04-29T18:32:59Z

fmriprep/workflows/bold/t2s.py

-            ('outputnode.skull_stripped_file', 'bold_ref_brain')]),
+        (t2smap_node, outputnode, [
+            ('optimal_comb', 'bold'),
+            ('t2star_map', 'bold_ref_brain')]),


This workflow is now inputnode -> t2smap_node -> outputnode, so the workflow itself can be replaced in the calling function by t2smap_node. Unless it's useful to keep this space open to expand the workflow in the future?

Also, bold_ref_brain does not seem to be being used anywhere.

I don't know if implementing it would require that bold_t2s_wf remain a workflow, but in #1010 (comment) I'm proposing a way to incorporate tedana's component classification into fMRIPrep. It might be worth it to keep the workflow around until #1010 is resolved at least.

I can remove bold_ref_brain now.

Sure. Let's keep this change minimal and we can reconsider architecture later. I realized there's also the workflow description that would need to be moved out of this separate module.

mgxd

we should also remove the config option:
https://github.com/poldracklab/fmriprep/blob/3cd56d5eaef6bd59c118116878841dcacfffc43b/fmriprep/config.py#L455-L456

tsalo · 2020-05-01T15:15:33Z

@mgxd thank you for catching that. It should be good now.

mgxd · 2020-05-01T15:25:45Z

LGTM, thanks!

oesteban · 2020-05-01T17:09:14Z

@effigies @mgxd merge? I'd like to see how #2108 is affected by this.

effigies · 2020-05-01T17:10:34Z

LGTM. Merge at will, @oesteban.

Shotgunosine · 2020-05-08T14:31:55Z

Sorry for the naive question, when would I expect to see this change in a release?

effigies · 2020-05-08T14:36:23Z

20.1.0, which is coming Real Soon Now.

Seriously, we're trying to wrap up as quickly as possible, but everything's taking longer than we intended.

Out of curiosity, is this a feature that you needed?

Shotgunosine · 2020-05-08T15:07:00Z

If I've followed all of the references right, I think this PR started with an issue I had with masking multi-echo rest sequences:
https://neurostars.org/t/registrations-and-brainmasks/4876/16

I had forgotten about it and ran fmriprepv20.0.5 without selecting a single echo (which was my work around) and found that a lot of the rest sequences have brain excluded from the brain mask around the base of the brain, which was the original issue:

I'm hoping that the new masking implemented here resolves this so that i can use the optimally combined rest instead of just a single echo.

Shotgunosine · 2020-05-08T15:38:26Z

@tsalo, I know I'm lat to the party here, but did you by any chance check to see if the masking here works on this dataset: https://openneuro.org/datasets/ds002156/versions/1.0.0

tsalo · 2020-05-08T17:43:37Z

@Shotgunosine I haven't checked the masking at all, sorry. I built this PR off of @emdupre's initial work in #1800, which I believe dealt with the masking more than what I added, which was just removing the t2s_coreg option from the workflow (and thus didn't touch non-t2s-coreg masking).

However, there was a glaring masking bug in the t2smap workflow in tedana <= 0.0.8 (see ME-ICA/tedana#544). I think that that bug could be responsible for at least some of the problems, and we should probably check how the masking works once 0.0.9 is released and I've pinned that version in fMRIPrep (see #2117).

Shotgunosine · 2020-05-08T18:16:24Z

Ok, sounds good. Looking at #1800 it seems like she verified that those changes worked with my data, so I'm optimistic that 20.1 will help when it gets here.

emdupre · 2020-05-08T18:20:02Z

In 04cf152 I moved all of the bold_mask generation steps outside of the T2* interface, so it should be using the same bold_mask that's generated from the SDC workflow. Which, in the case of multi-echo data, will be from the first echo rather than from the optimal combination ! So I don't think that the masking that you're mentioning @tsalo will really impact, here (but happy to discuss more out-of-band !).

Regardless, yes, I ran on yours locally and it looked right @Shotgunosine ! But fMRIPrep has had a lot of updates since then, so I'd be curious to re-run with a patched docker image for the current master.

effigies · 2020-05-08T18:40:50Z

To test with master:

fmriprep-docker -i poldracklab/fmriprep:unstable ...

Shotgunosine · 2020-05-20T19:05:21Z

I tested with unstable yesterday and it seems like it got much better masks on that test subject.

emdupre and others added 7 commits September 29, 2019 16:56

fix: remove bold_mask generation from t2s

04cf152

sty: appease flake8, retrigger CI

267171e

fix: pin nipype requirement on @effigies rec

40f1add

Merge branch 'master' into fix/t2s-wf

6689b4e

Merge remote-tracking branch 'origin' into fix/t2s-wf

62b91f0

Excise t2s-coreg.

d995645

Remove mention of t2s-coreg from workflow documentation.

4919d10

Fix bold_sdc_wf output name.

bf80c6d

emdupre mentioned this pull request Apr 29, 2020

[WIP, FIX] remove bold_mask generation from t2s #1800

Closed

tsalo added 4 commits April 28, 2020 21:29

Remove duplicate connection.

a8e4813

Drop t2s-coreg from test.

03b21da

Add bold_bold_trans_wf --> bold_confounds_wf connection back in.

ddedcd2

Grab BOLD mask from bold_bold_trans_wf.

56efef6

oesteban approved these changes Apr 29, 2020

View reviewed changes

effigies reviewed Apr 29, 2020

View reviewed changes

Remove unused output from init_bold_t2s_wf.

3a2f863

mgxd reviewed May 1, 2020

View reviewed changes

Address @mgxd's comment.

471fd9d

oesteban merged commit c0fe8b0 into nipreps:master May 1, 2020

tsalo deleted the enh/remove-t2s-coreg branch May 1, 2020 17:20

effigies mentioned this pull request May 5, 2020

MAINT: bump templateflow to auto-update skeleton #2115

Merged

tsalo mentioned this pull request May 5, 2020

[MAINT] Pin tedana version #2117

Merged

emdupre mentioned this pull request May 22, 2020

Improper outputs generated for ME-EPI #2134

Closed

emdupre mentioned this pull request Jun 6, 2020

[ME-EPI] Confirm that MELODIC is run on the optimally combined echo #2175

Closed

tsalo mentioned this pull request Jun 6, 2020

REF: Split final boldref generation from BOLD-BOLD resampling, eliminate extra per-echo computation #2181

Merged

tsalo mentioned this pull request Aug 17, 2020

Multiecho: accept SDC warpings in T2* calculation #1031

Closed

tsalo mentioned this pull request Oct 4, 2021

Multi-echo derivatives #2542

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Remove t2s-coreg #2109

ENH: Remove t2s-coreg #2109

tsalo commented Apr 29, 2020 •

edited

Loading

auto-comment bot commented Apr 29, 2020

oesteban left a comment

effigies Apr 29, 2020 •

edited

Loading

tsalo Apr 29, 2020

effigies Apr 29, 2020

mgxd left a comment

tsalo commented May 1, 2020

mgxd commented May 1, 2020

oesteban commented May 1, 2020

effigies commented May 1, 2020

Shotgunosine commented May 8, 2020

effigies commented May 8, 2020

Shotgunosine commented May 8, 2020

Shotgunosine commented May 8, 2020

tsalo commented May 8, 2020 •

edited

Loading

Shotgunosine commented May 8, 2020

emdupre commented May 8, 2020

effigies commented May 8, 2020

Shotgunosine commented May 20, 2020

ENH: Remove t2s-coreg #2109

ENH: Remove t2s-coreg #2109

Conversation

tsalo commented Apr 29, 2020 • edited Loading

Changes proposed in this pull request

Documentation that should be reviewed

auto-comment bot commented Apr 29, 2020

PR Review

Code documentation

Documentation site

Tests

Data

Dependencies: smriprep

Dependencies: niworkflows

Dependencies: sdcflows

Dependencies: Nipype

Dependencies: other

Reports generated within CI tests

oesteban left a comment

Choose a reason for hiding this comment

effigies Apr 29, 2020 • edited Loading

Choose a reason for hiding this comment

tsalo Apr 29, 2020

Choose a reason for hiding this comment

effigies Apr 29, 2020

Choose a reason for hiding this comment

mgxd left a comment

Choose a reason for hiding this comment

tsalo commented May 1, 2020

mgxd commented May 1, 2020

oesteban commented May 1, 2020

effigies commented May 1, 2020

Shotgunosine commented May 8, 2020

effigies commented May 8, 2020

Shotgunosine commented May 8, 2020

Shotgunosine commented May 8, 2020

tsalo commented May 8, 2020 • edited Loading

Shotgunosine commented May 8, 2020

emdupre commented May 8, 2020

effigies commented May 8, 2020

Shotgunosine commented May 20, 2020

tsalo commented Apr 29, 2020 •

edited

Loading

effigies Apr 29, 2020 •

edited

Loading

tsalo commented May 8, 2020 •

edited

Loading