nipreps / fmriprep

fMRIPrep is a robust and easy-to-use pipeline for preprocessing of diverse fMRI data. The transparent workflow dispenses of manual intervention, thereby ensuring the reproducibility of the results.
https://fmriprep.org
Apache License 2.0
612 stars 287 forks source link

BIDS Configuration (--bids-filter-file) for fieldmaps not respected #2336

Closed jerdra closed 1 year ago

jerdra commented 3 years ago

It looks like the specification for the fmap key enlisted in --bids-filter-file isn't used when pulling fmaps for a given BOLD file in init_func_preproc_wf. As a result all fmaps for the target BOLD file are used (even when the are incompatible with eachother, i.e different voxel resolutions) even when the BIDS config specifies that some files should be ignored.

What version of fMRIPrep are you using?

LTS 20.2.0

What kind of installation are you using? Containers (Singularity, Docker), or "bare-metal"?

Singularity

What is the exact command-line you used?

fmriprep /bids /output participant --participant_label $PLABEL \
    --bids-filter-file /resources/bids_configuration/spinecho_select.json \
    --nprocs 36 --omp-nthreads 8  -vv --output-spaces T1w \
    MNI152NLin2009cAsym fsaverage fsaverage5 \
    --bold2t1w-init register --bold2t1w-dof 6  \
    --use-syn-sdc --fs-license-file /license/license.txt

Have you checked that your inputs are BIDS valid?

Yes

The BIDS configuration file spinecho_select.json is configured as:

{
  "fmap": {"datatype": "fmap", "acquisition": null},
  "bold": {"datatype": "func", "suffix": "bold"},
  "sbref": {"datatype": "func", "suffix": "sbref"},
  "flair": {"datatype": "anat", "suffix": "FLAIR"},
  "t2w": {"datatype": "anat", "suffix": "T2w"},
  "t1w": {"datatype": "anat", "suffix": "T1w"},
  "roi": {"datatype": "anat", "suffix": "roi"}
}

I think the problematic line can be found here:

https://github.com/nipreps/fmriprep/blob/c707938e1462e77b1c3a1216a855ceb0aba70845/fmriprep/workflows/bold/base.py#L213-L215

Where fieldmap_wrangler doesn't know about the config.execution.bids_filter spec so it ends up pulling all files under "fmap".

I'd be more than happy to put together a small patch for this either in SDCFlows fieldmap_wrangler or possibly directly following the problematic line!

Let me know if that'd be useful for you, and how you'd like me to proceed!

bpinsard commented 3 years ago

Thanks for reporting that issue. So if I understand the bids contains multiple fieldmaps for the same acquisition (ie. multiple IntendedFor), this is uncommon but I see nothing against that in the BIDS specification. I suppose this was meant to offer multiple distortion correction options for the same data.

For the moment pybids.BIDSLayout.get_fieldmaps does not allow specifying additional filters to handle that case.

IMO the best fix would be to modify fmriprep, sdcflows and pybids to pass the additional filters. @oesteban WDYT?

To avoid waiting for these changes to be included in a new release, you can pre-index the BIDS dataset with pybids specifying ignore with regexp(s) for the fieldmaps you want to exclude and then run fmriprep passing the pre-indexed database path to --bids-database-dir.

oesteban commented 3 years ago

For the moment pybids.BIDSLayout.get_fieldmaps does not allow specifying additional filters to handle that case.

The new release of SDCFlows will not rely on get_fieldmaps - meaning, we need to come up with different solutions for the LTS and for 20.3+.

In addition, there's bids-standard/bids-specification#622 touching upon this very same issue. I think that, if the PR is finally merged, then PyBIDS will incorporate quite a bit of resources to better handle fieldmaps.

The confluence of both circumstances (the API breaking -or shattering- in SDCFlows and the update of BIDS/PyBIDS on fieldmaps) makes me lean towards marking this issue as wontfix.

jerdra commented 3 years ago

Thanks for reporting that issue. So if I understand the bids contains multiple fieldmaps for the same acquisition (ie. multiple IntendedFor), this is uncommon but I see nothing against that in the BIDS specification. I suppose this was meant to offer multiple distortion correction options for the same data.

For the moment pybids.BIDSLayout.get_fieldmaps does not allow specifying additional filters to handle that case.

IMO the best fix would be to modify fmriprep, sdcflows and pybids to pass the additional filters. @oesteban WDYT?

To avoid waiting for these changes to be included in a new release, you can pre-index the BIDS dataset with pybids specifying ignore with regexp(s) for the fieldmaps you want to exclude and then run fmriprep passing the pre-indexed database path to --bids-database-dir.

That's correct, we're currently running a study which happens to collect both SpinEcho and SBRef to use for distortion correction. We'd want to process the data both ways in order to get an idea of whether we can drop SpinEcho moving forward.

We decided against pre-indexing because we'd want to process the data as it comes in and re-indexing on every run would have been troublesome. However it looks like this isn't as simple as I imagined it would be w/the dependencies complicating the picture - we'll try to make @bpinsard's suggestion work for now.

Appreciate you taking the time to look into this!

It might be worth it to make a note of this somewhere maybe - given that the --bids-filter-file will not work as expected on fmaps?

oesteban commented 3 years ago

That's correct, we're currently running a study which happens to collect both SpinEcho and SBRef to use for distortion correction. We'd want to process the data both ways in order to get an idea of whether we can drop SpinEcho moving forward.

Please have a look into bids-standard/bids-specification#622 because I think it will definitely cover your current use-case (meaning, you are acquiring two types of fieldmap estimation approaches). Something interesting for you to look at (and which will only be possible after the big refactor we are making of SDCFlows) is whether you gain accuracy by estimating the fieldmap with both routes and then average the result.

Please note that I'm saying that addressing the problem modifying --bids-filters is not worth the pain for the LTS, in my opinion. That doesn't mean fMRIPrep should not support your use-case. On the contrary, with the new SDCFlows 1.4 I hope that fMRIPrep will soon be able to handle many sources of fieldmaps and combine them meaningfully. But the technical limitations of current settings and the inflexibility of the LTS in regards software dependencies makes this bug something that we might better let just be.

It might be worth it to make a note of this somewhere maybe

As I said, this will be addressed - but probably with a solution very different from --bids-filters

jerdra commented 3 years ago

Please have a look into bids-standard/bids-specification#622 because I think it will definitely cover your current use-case (meaning, you are acquiring two types of fieldmap estimation approaches)...

Thanks Oscar! I took a more thorough look at the discussion this time.. i was about to make a comment suggesting allowing for multiple B0FieldSources but you seem to be ahead of me!

Building off that, i understand this is specific to my rather uncommon use-case, but do you foresee pipelines like fMRIPrep/dMRIPrep being able to specify B0FieldSource identifiers as optional arguments to choose which fieldmap estimation technique to use (i.e if an array of B0FieldSources are spec'd in the side-car and you want to use only 1 of the B0FieldSources)?

Something interesting for you to look at (and which will only be possible after the big refactor we are making of SDCFlows) is whether you gain accuracy by estimating the fieldmap with both routes and then average the result.

This is an intriguing suggestion! I'll keep an eye out for that update to test it on my data when it drops!

oesteban commented 3 years ago

Building off that, i understand this is specific to my rather uncommon use-case, but do you foresee pipelines like fMRIPrep/dMRIPrep being able to specify B0FieldSource identifiers as optional arguments to choose which fieldmap estimation technique to use (i.e if an array of B0FieldSources are spec'd in the side-car and you want to use only 1 of the B0FieldSources)?

Yes, I definitely see that tools will need an interface to override the settings of the BIDS dataset. And I think this would be fine and useful oftentimes, IFF the tools also clearly report when and how the user made these customizations - so that the transparency and repeatability of what is done is ensured.

A related discussion (pertaining to the actual PR to the BIDS spec) is whether this B0FieldIdentifier / B0FieldSource could have more granularity to express even more circumstances. If you have any ideas/comments, please make sure to voice them there.

I'll keep an eye out for that update to test it on my data when it drops!

Well, I'm pushing hard - but this is a monster refactor so I can't make any promises.

effigies commented 2 years ago

Is this issue still of interest to anybody here?

jerdra commented 2 years ago

@effigies we've worked around this issue on our end! So not pressing for me personally.

I think maybe indicating this behaviour in documentation could be worthwhile if it's not already there?

djarecka commented 1 year ago

@effigies - I've just found this issue, since I'm having problems when running fmriprep for a specific session, my filter includes "fmap": {"datatype": "fmap", "session": "1year"}, but I still have fmap directory for other sessions.

effigies commented 1 year ago

@djarecka What version of fMRIPrep are you using? This should be resolved in 22.1.

djarecka commented 1 year ago

I've just double checked and I do have the same issue with 21.0.2 and 22.1.0

effigies commented 1 year ago

Could you share your whole filter file?

effigies commented 1 year ago

Oh, I see. We fixed it in sdcflows but haven't hooked it up here.

djarecka commented 1 year ago

filter file:

{
  "bold": {
    "datatype": "func",
    "suffix": "bold",
    "session": "baseline",
    "reconstruction": "unco"
  },
  "t1w": {
    "datatype": "anat",
    "suffix": "T1w",
    "session": "baseline"
  },
  "t2w": {
    "datatype": "anat",
    "suffix": "T2w",
    "session": "baseline"
  },
  "fmap": {
    "datatype": "fmap",
    "session": "baseline"
  }
}

in the output:

[10:44][13.87][-20%]djarecka@openmind7:sub-MM273$ tree -L 1 ses-*
ses-1year
└── fmap
ses-baseline
├── anat
├── fmap
└── func