nipreps / sdcflows

Susceptibility Distortion Correction (SDC) workflows for EPI MR schemes
https://www.nipreps.org/sdcflows
Apache License 2.0
32 stars 26 forks source link

IntendedFor is ignored by wrangler.find_estimators #266

Open bpinsard opened 2 years ago

bpinsard commented 2 years ago

I am trying the new fMRIPrep release 21.0.1, and I have never ending topup on pilot multi-echo data. Trying to investigate why, I found out that SDCFlow ignored the IntendedFor and topup is run on 20+ incongruent volumes (echoes with different distortions level from all possible epi fieldmap in the session).

Looking at the code, if I am not mistaken, it seems that the IntendedFor will never be used as the following code will take all EPI and return a single estimator per session. https://github.com/nipreps/sdcflows/blob/56a02bfa17a3640e91b4073ba4e5237bce19dcee/sdcflows/utils/wrangler.py#L291-L304 and then IntendedFor are skipped because the epi fmap is already used above. https://github.com/nipreps/sdcflows/blob/56a02bfa17a3640e91b4073ba4e5237bce19dcee/sdcflows/utils/wrangler.py#L313-L314

I would say that this issue is not multi-echo specific as it did not only aggregate echoes from the same series. The BIDS might be faulty as IntendedFor in multi-echo fMRI and phase-reversed EPI fmap is not detailled in the doc (I set it such that the each echo index _epi.json be IntendedFor the same echo index _bold.nii.gz). I could make changes to use the new B0FieldIdentifiers BIDS tags, but it would be good if that worked on legacy BIDS datasets using IntendedFor. Let me know if that's clear or if you need more info on the dataset.

bpinsard commented 2 years ago

Looking at the wrangler code, I can see that some options that were working in the previous version might no longer possible. For instance, running fmriprep on a dataset including fieldmaps with --use-syn-sdc to use Syn as fallback when fieldmaps are missing for some of the runs/sessions.

In addition, from the code logic it seems that if a dataset includes fieldmaps for DWI but not for BOLD, it might require using --force-syn to enable Syn on BOLD. As the wrangler is generic for both DWI/BOLD and fmriprep checks if any estimator has been returned, it could interact with the workflow setup.

https://github.com/nipreps/fmriprep/blob/6ba171f78cc4ece177b68e4953c83bc60c2c9071/fmriprep/workflows/base.py#L335-L341

bpinsard commented 2 years ago

Any potential fix for that issue? I am also puzzle by the pybids query to find the fmaps tagged with B0FieldIdentifier here https://github.com/nipreps/sdcflows/blob/83b064ee81760b03db3c4384510bafde78c79c06/sdcflows/utils/wrangler.py#L295-L299 as I cannot get any results with that query in a simple pybids example.

In [7]: layout.get_B0FieldIdentifiers()
['myb0id']
In [8]: layout.get(B0FieldIdentifier='myb0id')
[]
In [9]: fmap=layout.get(datatype='fmap',direction='AP',run=1, extension='.nii.gz',acquisition='sbref')[0]
In [11]: fmap.entities['B0FieldIdentifier']
Out[11]: ['myb0id']

Is querying files with metadata functional in pybids? Thank you in advance for any pointer.

bpinsard commented 2 years ago

I think I solved a part of the puzzle about pybids not finding the B0FieldIdentifier tagged files. If B0FieldIdentifier is a list in the json (which is allowed in the BIDS spec https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#echo-planar-imaging-and-b0-mapping ), then querying with a single B0FieldIdentifier string will of course not find any file exactly matching the query, which is what happens in https://github.com/nipreps/sdcflows/blob/83b064ee81760b03db3c4384510bafde78c79c06/sdcflows/utils/wrangler.py#L295-L299 . If I query layout.get(B0FieldIdentifier=bids.layout.Query.ANY) I do get the fieldmaps where that value is not empty. In fact it doesn't seem possible to query that an entity with list value contains a string from the BIDSLayout.get if I am not mistaken, so that won't be an easy fix. @mgxd @oesteban what do you think?

oesteban commented 1 year ago

I agree

layout.get(B0FieldIdentifier='myb0id')

should work out

bpinsard commented 1 year ago

You mean it should work with list, getting a match if any value in the list matches?

bpinsard commented 1 year ago

layout.get(B0FieldIdentifier='"myb0id"',regex_search=True) is able to find the files with the value contained in the list.

bpinsard commented 1 year ago

I can't wrap my head around how to make B0Field* work with sdcflows for pepolar with a single phase-reversed epi fmap. It seems that sdcflows is making assumptions about the scheme that are not set in the BIDS specs (which I convene is quite loose). https://bids-specification.readthedocs.io/en/stable/04-modality-specific-files/01-magnetic-resonance-imaging-data.html#echo-planar-imaging-and-b0-mapping

From the spec, one could set B0Fields in different ways to achieve the same goal which is for each bold find a pair of epis (_bold|_sbref|_epi) with opposite phase encoding direction (pedir):

The first way seems easier to fix. Let me know if that makes sense and what do you think about that issue. Thanks

effigies commented 1 year ago

@bpinsard did this get fully resolved?

bpinsard commented 1 year ago

I think the B0Field issues are resolved, but the problems with IntendedFor are not. I will try to find some time to have a small test case to replicate.