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

FIX: ME | Revise SDC compute graph #2610

Merged
merged 10 commits into from
Oct 22, 2021
Merged

FIX: ME | Revise SDC compute graph #2610

merged 10 commits into from
Oct 22, 2021

Conversation

oesteban
Copy link
Member

Although I brought up the issue within #2530 and got some mildly opposed initial opinion from @effigies (#2530 (comment)), I've come to think that we should provide the T2star workflow with HMC'ed and SDC'ed echos (confirmation awaiting: #2608 (review)).

This comes as an issue related to #2606.

This PR proposes that individual echoes are HMC'ed (and SDC'ed when fieldmaps are available) before calculating the optimal combination and the T2star map.

I believe this may be beneficial when we combine SDC with modulation by the Jacobian of the distortion (nipreps/sdcflows#238) because that might help restore some (little, admittedly) of the dropout of the later echoes in distorted regions. I would expect more voxels will be deemed as acceptable for the exponential fitting for this reason.

It also conceptually simplifies a little, since both SE and ME paths look more alike, and only the little hacks to generate ME iterables and the boldbuffer are now necessary. Otherwise, SDC would be inserted at a much later stage for ME only.

Although I brought up the issue within #2530 and got some mildly opposed initial
opinion from @effigies
(#2530 (comment)),
I've come to think that we should provide the T2star workflow with
HMC'ed and SDC'ed echos (confirmation awaiting:
#2608 (review)).

This comes as an issue related to #2606.

This PR proposes that individual echoes are HMC'ed (and SDC'ed when
fieldmaps are available) before calculating the optimal combination
and the T2star map.

I believe this may be beneficial when we combine SDC with modulation by
the Jacobian of the distortion (nipreps/sdcflows#238) because that might
help restore some (little, admittedly) of the dropout of the later
echoes in distorted regions. I would expect more voxels will be deemed
as acceptable for the exponential fitting for this reason.

It also conceptually simplifies a little, as both SE and ME paths look
more alike, and only the little hacks to generate ME iterables and the
``boldbuffer`` are now necessary. Otherwise, SDC would be inserted at a
much later stage for ME only.
@pep8speaks
Copy link

pep8speaks commented Oct 20, 2021

Hello @oesteban! 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 2021-10-22 12:32:04 UTC

@effigies
Copy link
Member

From @tsalo in #2542 (comment):

We currently recommend using non-SDCed data in tedana, but we don't have any actual tests comparing pre-SDC vs post-SDC. I have been using the SDCed data in my own analyses, because it's easier to take STC+MC+SDCed data, run it through tedana, and then apply the necessary transforms than to grab the data before SDC. If it's easy enough to run tedana within fMRIPrep before SDC, then I'd lean slightly toward that.

@oesteban
Copy link
Member Author

Sorry, I was unaware of the conversation. Moving it there.

@oesteban oesteban mentioned this pull request Oct 20, 2021
5 tasks
@effigies
Copy link
Member

@oesteban
Copy link
Member Author

Do we need to clear a cache?

It's possible that it's just that. This problem is what I meant before by needing to revise the generation of the reference. With the original ds210 from ON registration is fine

@effigies
Copy link
Member

b67636b was in response to the error:

211021-17:53:27,719 nipype.workflow WARNING:
	 [Node] Error on "fmriprep_wf.single_subject_02_wf.func_preproc_task_cuedSGT_run_01_echo_1_wf.join_echos" (/scratch/fmriprep_wf/single_subject_02_wf/func_preproc_task_cuedSGT_run_01_echo_1_wf/join_echos)
211021-17:53:27,726 nipype.workflow ERROR:
	 Node join_echos failed to run on host 11952ca71203.
211021-17:53:27,743 nipype.workflow ERROR:
	 Saving crash info to /out/fmriprep/sub-02/log/20211021-175101_e7b572c4-16da-4f88-b7de-ea8f6e54c632/crash-20211021-175327-UID1001-join_echos-4041e345-64ab-4edd-91fc-bbfe13263964.txt
Traceback (most recent call last):
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/plugins/multiproc.py", line 67, in run_node
    result["result"] = node.run(updatehash=updatehash)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 516, in run
    result = self._run_interface(execute=True)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 635, in _run_interface
    return self._run_command(execute)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 1004, in _run_command
    return super(JoinNode, self)._run_command(execute, copyfiles)
  File "/opt/conda/lib/python3.8/site-packages/nipype/pipeline/engine/nodes.py", line 741, in _run_command
    result = self._interface.run(cwd=outdir)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 430, in run
    outputs = self.aggregate_outputs(runtime)
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/base/core.py", line 506, in aggregate_outputs
    predicted_outputs = self._list_outputs()  # Predictions from _list_outputs
  File "/opt/conda/lib/python3.8/site-packages/nipype/interfaces/utility/base.py", line 92, in _list_outputs
    raise ValueError(msg)
ValueError: IdentityInterface requires a value for input 'bold_masks' because it was listed in 'fields'.                     You can turn off mandatory inputs checking by passing mandatory_inputs = False to the constructor.

Possibly setting mandatory_inputs=False would have been the better fix. I don't think we are passing that value out in that case...

effigies and others added 2 commits October 21, 2021 16:30
Turns out that our bold-to-bold resampling workflow has fake inputs and
outputs called ``bold_mask``.
That made me believe that, if we were joining the bold echoes from the
bold-to-bold resampling workflow, we could do the same with the mask.

Looking into this in depth made me realize of the confusing i/o spec of
the workflow, and also of the fact that it is unnecessary to join
anything in the absence of fieldwarp because the mask is the same you
calculated with the initial boldref.
So I connected that one and locally seems to be functioning okay.

Hopefully, this was the last bit of this not-so-trivial attempt to
organize the ME-EPI pathway. I'm already shaking by thinking at some
point we will need a similar in-depth revision of the SBRef dealings...
@oesteban
Copy link
Member Author

Actively working on this - please do not push more commits

@oesteban
Copy link
Member Author

Alright, I had to do this nipy/nipype#3395 in order to find out this:

mask_file = File(argstr='--mask',

(argstr is missing the %s replacement tag).

Basically, our T2map interface fails always if given a mask file.

@oesteban
Copy link
Member Author

Okay, this is working locally. Perhaps, the final question before going ahead (should tests be all green) is the following:

Here I'm connecting a brain mask into the tedana node:

  • We definitely were not passing it in before (otherwise, the interface coding error would have emerged earlier)
  • It produces brain-masked results (which is understandable). Do we want this?

boldbuffer = pe.Node(niu.IdentityInterface(fields=["bold_file", "name_source"]),
name="boldbuffer")
if multiecho:
boldbuffer.synchronize = True
Copy link
Member

Choose a reason for hiding this comment

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

I forget what this does. Can look it up when there's time, or if you want to say a quick word?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. Because boldbuffer now has two fields, and both are iterable, synchronize = True preempts the dot product of the values of the two iterable inputs.

@effigies
Copy link
Member

Overall looks good. The final brainmask looks very liberal:

@oesteban
Copy link
Member Author

Overall looks good. The final brainmask looks very liberal:

Yes. However, I think we should merge this and come back to the mask at a later time. We might want to calculate the final boldref from the optimal combination of echos to open a window into the OC. We will probably need a simplified calculation (perhaps just the robust average without running hmc, as the echos were HMC'd already). Because these changes seem a bit involved, we are probably better not including them here. WDTY?

@oesteban
Copy link
Member Author

Let's go for it

@oesteban oesteban merged commit 672d071 into master Oct 22, 2021
@oesteban oesteban deleted the fix/sdc-and-me branch October 22, 2021 15:57
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