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

tsalo · 2020-06-06T18:46:13Z

Closes #2175. It appears that dropping the tedana-based mask in #2109 led to un-joined echo-specific masks being used after echo combination throughout the workflow. This led to many redundant nodes in the workflow being created, including multiple runs of ICA-AROMA, although those nodes should be duplicates of one another except for the difference, if any, in masks.

Changes proposed in this pull request

Move final reference image creation step out of init_bold_preproc_trans_wf and into init_func_preproc_wf. The workflow is called final_boldref_wf.
Rename bold_reference_wf in init_func_preproc_wf to initial_boldref_wf.
Replace bold_files in join_echos JoinNode with reference native-space BOLD files from bold_bold_trans_wf.
Add skullstripped_bold_files to join_echos JoinNode, to combine skullstripped BOLD files from skullstrip_bold_wf and feed them to bold_t2s_wf. (These were the bold_files in join_echos before.)

Documentation that should be reviewed

None

oesteban

Should we rebase this PR on the head of #1803? I think yielding until that one is merged will help clarify this.

oesteban · 2020-06-07T06:56:46Z

fmriprep/utils/misc.py

@@ -11,3 +11,22 @@ def check_deps(workflow):
        for node in workflow._get_all_nodes()
        if (hasattr(node.interface, '_cmd') and
            which(node.interface._cmd.split()[0]) is None))
+
+
+def select_first(in_files):


Let's use niworkflows.utils.connections.pop_file instead (requires nipreps/niworkflows#408 to be merged)

Do we want to use the first echo's brain mask, though? I am using it to fix the bug, but I don't know if it's the appropriate choice.

I think we can also replace fmriprep.workflows.bold.resampling._first with pop_file as well.

# Conflicts: # fmriprep/utils/misc.py

oesteban

Okay, I see the problem now. Because we run the bold resampling workflow as an iterable, we are generating one mask per echo, regardless of the changes in niworkflows to use the first echo only.

Although this looks like it fixes the problem, I believe that we are running some unnecessary stuff in the bold resampling workflow we should avoid (starting with the masking of each echo).

Please allow me some time to draft how the bold transform workflow could be improved and send a PR to your branch.

fmriprep/workflows/bold/base.py

effigies · 2020-08-14T03:07:26Z

Not to step on @oesteban's toes, but it looks like this would be good to get into the next release, if it's not too far from ready. I can plan to review tomorrow.

@tsalo Would you be willing to rebase on top of the refactors in #2239? Sorry to do this to you, but I think we've almost certainly interfered with your changes.

effigies · 2020-08-15T02:03:09Z

Test failures are related to nipreps/niworkflows#556.

effigies · 2020-08-17T02:27:15Z

Tests fixed.

effigies

Thanks for this. The connections look good. I also see what @oesteban was saying, which is that we are calculating new masks for each echo, and throwing away all but the first:

https://github.com/poldracklab/fmriprep/blob/5d8fe4880cbdd2e2ac7fffb7db0740adbfdcedfe/fmriprep/workflows/bold/resampling.py#L518-L520

The simple solution is to remove this from init_bold_preproc_trans_wf, and put it directly into init_func_preproc_wf, connecting from init_bold_preproc_trans_wf.outputs.outputnode.bold or join_echos.outputs.bold_files. There might be a more clever way to do it without moving the workflow, but I'm not sure that cleverness would be worth it.

WDYT?

fmriprep/workflows/bold/base.py

tsalo · 2020-08-17T16:17:32Z

The simple solution is to remove this from init_bold_preproc_trans_wf, and put it directly into init_func_preproc_wf, connecting from init_bold_preproc_trans_wf.outputs.outputnode.bold or join_echos.outputs.bold_files. There might be a more clever way to do it without moving the workflow, but I'm not sure that cleverness would be worth it.

To clarify, init_func_preproc_wf has an initial bold_reference_wf even before STC. As part of the reference image estimation, there is a quick HMC, but not STC or SDC, right?

Then, I think the one in init_bold_preproc_trans_wf is calculated after STC+HMC+SDC.

If so, then we could label these as "first pass" and "second pass" reference image estimation, correct?

In any case, I think that moving the second run of the reference workflow to init_func_preproc_wf makes sense.

effigies · 2020-08-17T16:47:13Z

If so, then we could label these as "first pass" and "second pass" reference image estimation, correct?

Yes, that makes sense to me. I might call them initial_boldref_wf and final_boldref_wf, rather than leave ambiguity as to whether to expect a third pass, but as long as it's clear, I'm happy.

tsalo · 2020-08-17T16:50:29Z

Awesome! I think I can handle the refactor, but would it be better to have it here or in a separate PR?

Co-authored-by: Chris Markiewicz <[email protected]>

effigies · 2020-08-17T16:54:22Z

I don't think it makes any difference to me. I would just have a look and see how much of this PR would need to be undone, and whether it makes sense to start fresh from master or keep going here.

pep8speaks · 2020-08-17T17:39:01Z

Hello @tsalo! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-18 13:39:55 UTC

tsalo · 2020-08-17T17:41:16Z

@effigies. Okay, I think I got it working. It did involve switching a lot of the stuff in the PR back, but I don't think that's too much of a problem. We'll see how the tests do.

effigies · 2020-08-17T21:02:05Z

Crashed...

200817-20:37:33,217 nipype.workflow INFO:
	 [Node] Setting-up "fmriprep_wf.single_subject_02_wf.func_preproc_task_cuedSGT_run_01_echo_1_wf.bold_t2smap_wf.t2smap_node" in "/scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/bold_t2smap_wf/t2smap_node".
exception calling callback for <Future at 0x7ff94ab8d0b8 state=finished raised FileNotFoundError>
concurrent.futures.process._RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/plugins/multiproc.py", line 67, in run_node
    result["result"] = node.run(updatehash=updatehash)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 486, in run
    self._get_hashval()
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 538, in _get_hashval
    self._get_inputs()
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 609, in _get_inputs
    self.set_input(key, deepcopy(output_value))
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 302, in set_input
    setattr(self.inputs, parameter, deepcopy(val))
  File "/usr/local/miniconda/lib/python3.7/site-packages/traits/trait_types.py", line 2338, in validate
    self.error( object, name, value )
  File "/usr/local/miniconda/lib/python3.7/site-packages/traits/trait_handlers.py", line 172, in error
    value )
traits.trait_errors.TraitError: The 'in_files' trait of a T2SMapInputSpec instance must be a list of at least 3 items which are a pathlike object or string representing an existing file, but a value of '/scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/skullstrip_bold_wf/_bold_file_..data..sub-02..func..sub-02_task-cuedSGT_run-01_echo-3_bold.nii.gz/apply_mask/vol0000_xform-00000_merged_masked.nii.gz' <class 'str'> was specified.

Error setting node input:
Node: t2smap_node
input: in_files
results_file: /scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/join_echos/result_join_echos.pklz
value: /scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/skullstrip_bold_wf/_bold_file_..data..sub-02..func..sub-02_task-cuedSGT_run-01_echo-3_bold.nii.gz/apply_mask/vol0000_xform-00000_merged_masked.nii.gz

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/concurrent/futures/process.py", line 232, in _process_worker
    r = call_item.fn(*call_item.args, **call_item.kwargs)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/plugins/multiproc.py", line 70, in run_node
    result["result"] = node.result
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/nodes.py", line 217, in result
    op.join(self.output_dir(), "result_%s.pklz" % self.name)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/engine/utils.py", line 291, in load_resultfile
    raise FileNotFoundError(results_file)
FileNotFoundError: /scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/bold_t2smap_wf/t2smap_node/result_t2smap_node.pklz
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/miniconda/lib/python3.7/concurrent/futures/_base.py", line 324, in _invoke_callbacks
    callback(self)
  File "/usr/local/miniconda/lib/python3.7/site-packages/nipype/pipeline/plugins/multiproc.py", line 159, in _async_callback
    result = args.result()
  File "/usr/local/miniconda/lib/python3.7/concurrent/futures/_base.py", line 425, in result
    return self.__get_result()
  File "/usr/local/miniconda/lib/python3.7/concurrent/futures/_base.py", line 384, in __get_result
    raise self._exception
FileNotFoundError: /scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/bold_t2smap_wf/t2smap_node/result_t2smap_node.pklz

The issue isn't obvious to me. Could just be a cache that needs clearing.

fmriprep/workflows/bold/base.py

emdupre · 2020-08-18T00:56:16Z

Could just be a cache that needs clearing.

I think it's a genuine error... The T2* workflow expects a list (with a minimum of 3 entries), but a string is being passed. I wonder if join_echos now needs another JoinField

effigies · 2020-08-18T01:04:10Z

Good catch. JoinNodes.

Co-authored-by: Chris Markiewicz <[email protected]>

fmriprep/workflows/bold/base.py

Co-authored-by: Chris Markiewicz <[email protected]>

effigies · 2020-08-18T12:57:22Z

Okay, looks like this is working. Nice! I'll review for aesthetic/style stuff, and I think we can call this done.

effigies

Mostly suggestions to compact the diff. One verification.

Would you rather we merge by squashing or merge commit?

fmriprep/workflows/bold/base.py

effigies · 2020-08-18T13:07:30Z

fmriprep/workflows/bold/base.py

                ('outputnode.bold_mask', 'inputnode.bold_mask_native')]),
+            (bold_bold_trans_wf if not multiecho else bold_t2s_wf, outputnode, [


This seems to be a semantic change, not just a reorganization. Was this something we were doing wrong?

Sorry, yes, I think it was wrong before, since it took the BOLD files from bold_bold_trans_wf for the native-space BOLD outputs.

Looking through, I agree with this change. If I'm reading it right, a native BOLD space derivative for MEEPI will be the optimal combination, which seems appropriate.

fmriprep/workflows/bold/base.py

@effigies

Thanks @effigies. Co-authored-by: Chris Markiewicz <[email protected]>

tsalo · 2020-08-18T13:24:32Z

Generally speaking, I prefer squash and merge. It's a fairly cohesive set of changes, so the individual commits don't provide any extra information, IMHO. Especially given the pivot in approach.

fmriprep/workflows/bold/base.py

effigies

LGTM. We'll let the CI run through one more time to be safe, but I think we're good to go, and anybody with permissions should feel free to squash and merge.

tsalo added 2 commits June 5, 2020 23:54

Join bold masks and select first echo's mask for multi-echo data.

abee610

Fix and consolidate multi-echo-toggled connections.

cd1925d

tsalo mentioned this pull request Jun 6, 2020

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

Closed

oesteban reviewed Jun 7, 2020

View reviewed changes

Shotgunosine mentioned this pull request Jun 8, 2020

ICA-AROMA attempted to run on several echoes: not enough values to unpack #2153

Closed

tsalo added 2 commits June 8, 2020 20:48

Merge remote-tracking branch 'poldracklab/master' into fix/meepi-aroma

559e11c

# Conflicts: # fmriprep/utils/misc.py

Replace select_first with pop_file.

a5e387d

tsalo marked this pull request as ready for review June 9, 2020 14:44

oesteban reviewed Jun 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Show resolved Hide resolved

tsalo added 3 commits August 14, 2020 21:39

Merge branch 'master' into fix/meepi-aroma

0b8a388

Add freesurfer check back in.

d3d3dab

Fix up merge.

5d8fe48

effigies reviewed Aug 17, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

Apply suggestions from code review

211c298

Co-authored-by: Chris Markiewicz <[email protected]>

Move final boldref wf to init_func_preproc_wf.

309a613

Fix the bugs.

80aa3b6

tsalo added 2 commits August 17, 2020 14:14

Fix links.

0f8936a

Fix reference image link.

9e5a689

emdupre reviewed Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

Update fmriprep/workflows/bold/base.py

71226b9

Co-authored-by: Chris Markiewicz <[email protected]>

effigies reviewed Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Show resolved Hide resolved

effigies reviewed Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

Update fmriprep/workflows/bold/base.py

9d530ea

Co-authored-by: Chris Markiewicz <[email protected]>

effigies reviewed Aug 18, 2020

View reviewed changes

Apply suggestions from code review

3703963

Thanks @effigies. Co-authored-by: Chris Markiewicz <[email protected]>

tsalo changed the title ~~FIX: Use first echo's mask for multi-echo data~~ FIX, REF: Move final reference image creation out of init_bold_preproc_trans_wf Aug 18, 2020

tsalo changed the title ~~FIX, REF: Move final reference image creation out of init_bold_preproc_trans_wf~~ REF: Move final reference image creation out of init_bold_preproc_trans_wf Aug 18, 2020

tsalo commented Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

Update fmriprep/workflows/bold/base.py

3e9d2cb

tsalo commented Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

tsalo commented Aug 18, 2020

View reviewed changes

fmriprep/workflows/bold/base.py Outdated Show resolved Hide resolved

Cleanup to minimize diff.

7415cbb

effigies approved these changes Aug 18, 2020

View reviewed changes

effigies changed the title ~~REF: Move final reference image creation out of init_bold_preproc_trans_wf~~ REF: Split final boldref generation from BOLD-BOLD resampling, eliminate extra per-echo computation Aug 18, 2020

tsalo merged commit 726584d into nipreps:master Aug 18, 2020

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

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

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

tsalo commented Jun 6, 2020 •

edited

Loading

oesteban left a comment

oesteban Jun 7, 2020

tsalo Jun 7, 2020

tsalo Jun 9, 2020

oesteban left a comment

effigies commented Aug 14, 2020

effigies commented Aug 15, 2020

effigies commented Aug 17, 2020

effigies left a comment

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

pep8speaks commented Aug 17, 2020 •

edited

Loading

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

emdupre commented Aug 18, 2020

effigies commented Aug 18, 2020

effigies commented Aug 18, 2020

effigies left a comment

effigies Aug 18, 2020

tsalo Aug 18, 2020

effigies Aug 18, 2020

tsalo commented Aug 18, 2020

effigies left a comment

		('outputnode.bold_mask', 'inputnode.bold_mask_native')]),
		(bold_bold_trans_wf if not multiecho else bold_t2s_wf, outputnode, [

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

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

Conversation

tsalo commented Jun 6, 2020 • edited Loading

Changes proposed in this pull request

Documentation that should be reviewed

oesteban left a comment

Choose a reason for hiding this comment

oesteban Jun 7, 2020

Choose a reason for hiding this comment

tsalo Jun 7, 2020

Choose a reason for hiding this comment

tsalo Jun 9, 2020

Choose a reason for hiding this comment

oesteban left a comment

Choose a reason for hiding this comment

effigies commented Aug 14, 2020

effigies commented Aug 15, 2020

effigies commented Aug 17, 2020

effigies left a comment

Choose a reason for hiding this comment

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

pep8speaks commented Aug 17, 2020 • edited Loading

Comment last updated at 2020-08-18 13:39:55 UTC

tsalo commented Aug 17, 2020

effigies commented Aug 17, 2020

emdupre commented Aug 18, 2020

effigies commented Aug 18, 2020

effigies commented Aug 18, 2020

effigies left a comment

Choose a reason for hiding this comment

effigies Aug 18, 2020

Choose a reason for hiding this comment

tsalo Aug 18, 2020

Choose a reason for hiding this comment

effigies Aug 18, 2020

Choose a reason for hiding this comment

tsalo commented Aug 18, 2020

effigies left a comment

Choose a reason for hiding this comment

tsalo commented Jun 6, 2020 •

edited

Loading

pep8speaks commented Aug 17, 2020 •

edited

Loading