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

fmriprep output report does not properly handle multiple resting state runs that differ only in contrast #2033

Closed
bbfrederick opened this issue Mar 12, 2020 · 21 comments
Labels

Comments

@bbfrederick
Copy link
Contributor

bbfrederick commented Mar 12, 2020

fmriprep version 20.0.2

I ran an analysis to completion (no errors) that contained a T1w anatomic image and two resting state runs taken in the same session, one before Gd contrast was injected, and one several minutes after. The subject folder was as follows:

sub-B13293
├── anat
│   ├── sub-B13293_T1w.json
│   └── sub-B13293_T1w.nii.gz
└── func
    ├── sub-B13293_task-rest_ce-Gd_bold.json
    ├── sub-B13293_task-rest_ce-Gd_bold.nii.gz
    ├── sub-B13293_task-rest_ce-none_bold.json
    └── sub-B13293_task-rest_ce-none_bold.nii.gz

As far as I can tell, both files were analyzed properly, and created all the appropriate outputs in the anat and func folders of derivatives/fmriprep /sub-B13293. The output files for ce-none and cd-Gd are different, and appear to reflect the actual input files.

sub-B13293
├── anat
│   ├── sub-B13293_desc-brain_mask.json
│   ├── sub-B13293_desc-brain_mask.nii.gz
│   ├── sub-B13293_desc-preproc_T1w.json
│   ├── sub-B13293_desc-preproc_T1w.nii.gz
│   ├── sub-B13293_dseg.nii.gz
│   ├── sub-B13293_from-MNI152NLin2009cAsym_to-T1w_mode-image_xfm.h5
│   ├── sub-B13293_from-MNI152NLin6Asym_to-T1w_mode-image_xfm.h5
│   ├── sub-B13293_from-T1w_to-MNI152NLin2009cAsym_mode-image_xfm.h5
│   ├── sub-B13293_from-T1w_to-MNI152NLin6Asym_mode-image_xfm.h5
│   ├── sub-B13293_label-CSF_probseg.nii.gz
│   ├── sub-B13293_label-GM_probseg.nii.gz
│   ├── sub-B13293_label-WM_probseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_desc-brain_mask.json
│   ├── sub-B13293_space-MNI152NLin2009cAsym_desc-brain_mask.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_desc-preproc_T1w.json
│   ├── sub-B13293_space-MNI152NLin2009cAsym_desc-preproc_T1w.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_dseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_label-CSF_probseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_label-GM_probseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin2009cAsym_label-WM_probseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin6Asym_desc-brain_mask.json
│   ├── sub-B13293_space-MNI152NLin6Asym_desc-brain_mask.nii.gz
│   ├── sub-B13293_space-MNI152NLin6Asym_desc-preproc_T1w.json
│   ├── sub-B13293_space-MNI152NLin6Asym_desc-preproc_T1w.nii.gz
│   ├── sub-B13293_space-MNI152NLin6Asym_dseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin6Asym_label-CSF_probseg.nii.gz
│   ├── sub-B13293_space-MNI152NLin6Asym_label-GM_probseg.nii.gz
│   └── sub-B13293_space-MNI152NLin6Asym_label-WM_probseg.nii.gz
├── figures
│   ├── sub-B13293_desc-reconall_T1w.svg
│   ├── sub-B13293_dseg.svg
│   ├── sub-B13293_space-MNI152NLin2009cAsym_T1w.svg
│   ├── sub-B13293_space-MNI152NLin6Asym_T1w.svg
│   ├── sub-B13293_task-rest_desc-aroma_bold.svg
│   ├── sub-B13293_task-rest_desc-bbregister_bold.svg
│   ├── sub-B13293_task-rest_desc-carpetplot_bold.svg
│   ├── sub-B13293_task-rest_desc-compcorvar_bold.svg
│   ├── sub-B13293_task-rest_desc-confoundcorr_bold.svg
│   ├── sub-B13293_task-rest_desc-flirtbbr_bold.svg
│   ├── sub-B13293_task-rest_desc-rois_bold.svg
│   └── sub-B13293_task-rest_desc-sdc_bold.svg
├── func
│   ├── sub-B13293_task-rest_ce-Gd_AROMAnoiseICs.csv
│   ├── sub-B13293_task-rest_ce-Gd_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_desc-MELODIC_mixing.tsv
│   ├── sub-B13293_task-rest_ce-Gd_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-Gd_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_desc-confounds_regressors.json
│   ├── sub-B13293_task-rest_ce-Gd_desc-confounds_regressors.tsv
│   ├── sub-B13293_task-rest_ce-Gd_desc-preproc_bold.json
│   ├── sub-B13293_task-rest_ce-Gd_desc-preproc_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_desc-smoothAROMAnonaggr_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_res-2_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_res-2_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_res-2_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_res-2_desc-preproc_bold.json
│   ├── sub-B13293_task-rest_ce-Gd_space-MNI152NLin6Asym_res-2_desc-preproc_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-T1w_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-T1w_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-Gd_space-T1w_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-Gd_space-T1w_desc-preproc_bold.json
│   ├── sub-B13293_task-rest_ce-Gd_space-T1w_desc-preproc_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-none_AROMAnoiseICs.csv
│   ├── sub-B13293_task-rest_ce-none_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-none_desc-MELODIC_mixing.tsv
│   ├── sub-B13293_task-rest_ce-none_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-none_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-none_desc-confounds_regressors.json
│   ├── sub-B13293_task-rest_ce-none_desc-confounds_regressors.tsv
│   ├── sub-B13293_task-rest_ce-none_desc-preproc_bold.json
│   ├── sub-B13293_task-rest_ce-none_desc-preproc_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_desc-smoothAROMAnonaggr_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_res-2_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_res-2_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_res-2_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_res-2_desc-preproc_bold.json
│   ├── sub-B13293_task-rest_ce-none_space-MNI152NLin6Asym_res-2_desc-preproc_bold.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-T1w_boldref.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-T1w_desc-brain_mask.json
│   ├── sub-B13293_task-rest_ce-none_space-T1w_desc-brain_mask.nii.gz
│   ├── sub-B13293_task-rest_ce-none_space-T1w_desc-preproc_bold.json
│   └── sub-B13293_task-rest_ce-none_space-T1w_desc-preproc_bold.nii.gz
└── log

However as you can see in the figures folder, only one set of functional figures was produced - sub-B13293_task-rest_desc*. This matches the html output file - it says there are two input task-rest files, and shows two sets of images for the registration, confounds, and aroma analysis - but they are identical. I've done this with datsets that have multiple _task-rest_run_X files, and those are handled properly - the report shows them as unique files. But somewhere in the generation of the report and figures, it seems to throw away the "ce" BIDS entity and treat the files the same.

@bbfrederick
Copy link
Contributor Author

bbfrederick commented Mar 12, 2020

I'm wondering if this is actually a niworkflows bug. Poking around in the fmriprep/interfaces/reports.py line 123 it looks like it gets the information from BIDS_NAME:

        counts = Counter(BIDS_NAME.search(series).groupdict()['task_id'][5:]
                         for series in bold_series)

So that will give the correct number of files, but in niworkflows.utils.bids:

BIDS_NAME = re.compile(
    r'^(.*\/)?'
    '(?P<subject_id>sub-[a-zA-Z0-9]+)'
    '(_(?P<session_id>ses-[a-zA-Z0-9]+))?'
    '(_(?P<task_id>task-[a-zA-Z0-9]+))?'
    '(_(?P<acq_id>acq-[a-zA-Z0-9]+))?'
    '(_(?P<rec_id>rec-[a-zA-Z0-9]+))?'
    '(_(?P<run_id>run-[a-zA-Z0-9]+))?')

BIDS_NAME seems to ignore the ce entity, so the two entries will not be distinct - whichever file it happens to hit first in search will be returned each time. Would fixing this line fix the problem?

@effigies
Copy link
Member

Certainly we should accept the ce entity, but I can't say for certain that it's the only place we're neglecting it, so this bug may originate elsewhere. Are you comfortable patching and testing?

@bbfrederick
Copy link
Contributor Author

I'm certainly up for giving it a go. I'll go fork niworkflows and poke around.

@oesteban
Copy link
Member

@jdkent can surely help you with this.

@bbfrederick
Copy link
Contributor Author

bbfrederick commented Mar 17, 2020

I have a little more information on this now - testing this is slow, especially since every time I change the code, all the caches become invalid and it wants to run the entire analysis again (and report generation is the last step!)

As is often the case when debugging, I'm now in the "how did this ever work?" phase. I've fixed BIDS_NAME to find ce_id, dir_id, and echo_id (the tags that were not being parsed), but I don't see anywhere that anything other than the task ID (no run_id or anything) gets passed on to the report generator. It does infer the number of runs (by counting the number of series that match the task_id), but does not use the specific run_id tag anywhere. So the question now is "how is it able to differentiate the runs?"

@effigies
Copy link
Member

@bbfrederick If you're using Docker, use fmriprep-docker -f $FMRIPREP_REPO/fmriprep -n $NIWORKFLOWS_REPO/niworkflows to patch these into an already-built container.

Also, one thing you'll probably need to do is update:

diff --git a/niworkflows/reports/figures.json b/niworkflows/reports/figures.json
index 7122243d0..4551f1786 100644
--- a/niworkflows/reports/figures.json
+++ b/niworkflows/reports/figures.json
@@ -113,6 +113,6 @@
 
     "default_path_patterns": [
         "sub-{subject}[/ses-{session}]/{datatype<anat|figures>}/sub-{subject}[_ses-{session}][_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}][_space-{space}][_desc-{desc}]_{suffix<T1w|T2w|T1rho|T1map|T2map|T2star|FLAIR|FLASH|PDmap|PD|PDT2|inplaneT[12]|angio|dseg|mask>}.{extension<html|svg>}",
-        "sub-{subject}[/ses-{session}]/{datatype<func|figures>}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_rec-{reconstruction}][_run-{run}][_echo-{echo}][_space-{space}][_desc-{desc}]_{suffix<bold>}.{extension<html|svg>}"
+        "sub-{subject}[/ses-{session}]/{datatype<func|figures>}/sub-{subject}[_ses-{session}]_task-{task}[_acq-{acquisition}][_ce-{contrast}][_rec-{reconstruction}][_run-{run}][_echo-{echo}][_space-{space}][_desc-{desc}]_{suffix<bold>}.{extension<html|svg>}"
     ]
 }

This should allow CE agent to be added to the figure. It's possible that this is all that will be necessary for the report generator to figure it out.

@jdkent
Copy link
Collaborator

jdkent commented Mar 17, 2020

Hi @bbfrederick, one more thing to help isolate this bug, inside the working directory there should be a directory named reportlets. Could you give the tree output for the participant sub-B13293 within the reportlets directory.

This will help differentiate: A) the situation where the ce- svg/html files are not being created, versus B) the svg files are created, but are incorrectly processed in some way for the report creation.

Thank you for debugging this!

@bbfrederick
Copy link
Contributor Author

bbfrederick commented Mar 17, 2020

Ah! It's B.

[bbf-mbp16:MR_data/workdirs/prepostgad] frederic% tree reportlets
reportlets
└── fmriprep
    └── sub-B13293
        ├── anat
        │   ├── sub-B13293_desc-about_T1w.html
        │   ├── sub-B13293_desc-conform_T1w.html
        │   ├── sub-B13293_desc-summary_T1w.html
        │   ├── sub-B13293_dseg.svg
        │   ├── sub-B13293_space-MNI152NLin2009cAsym_T1w.svg
        │   └── sub-B13293_space-MNI152NLin6Asym_T1w.svg
        └── func
            ├── sub-B13293_task-rest_ce-Gd_desc-aroma_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-carpetplot_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-compcorvar_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-confoundcorr_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-flirtbbr_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-rois_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-sdc_bold.svg
            ├── sub-B13293_task-rest_ce-Gd_desc-summary_bold.html
            ├── sub-B13293_task-rest_ce-Gd_desc-validation_bold.html
            ├── sub-B13293_task-rest_ce-none_desc-aroma_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-carpetplot_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-compcorvar_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-confoundcorr_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-flirtbbr_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-rois_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-sdc_bold.svg
            ├── sub-B13293_task-rest_ce-none_desc-summary_bold.html
            └── sub-B13293_task-rest_ce-none_desc-validation_bold.html

4 directories, 24 files

I'm currently rerunning the analysis the way Chris suggested, after fixing the figures.json files.

@jdkent
Copy link
Collaborator

jdkent commented Mar 18, 2020

ah, I think this needs to be changed in figures.json to be in line with pybids, that is at least one of the places.

diff --git a/niworkflows/reports/figures.json b/niworkflows/reports/figures.json
index 7122243..866d716 100644
--- a/niworkflows/reports/figures.json
+++ b/niworkflows/reports/figures.json
@@ -21,7 +21,7 @@
             "pattern": "[_/\\\\]acq-([a-zA-Z0-9]+)"
         },
         {
-            "name": "ce",
+            "name": "ceagent",
             "pattern": "[_/\\\\]ce-([a-zA-Z0-9]+)"
         },
         {

@jdkent
Copy link
Collaborator

jdkent commented Mar 18, 2020

@bbfrederick, I think I have the fix for your situation, could you try running (yet again), with my branch/fork of niworkflows? (unless if your fix works, then I will need to re-evaluate).

And to help answer your question about how did this work in the first place, you should take a look in fmriprep.yml to see that fmriprep is looking at the different keys of a bids file.

@bbfrederick
Copy link
Contributor Author

I tried running your branch, and it complains:

Status: Downloaded newer image for poldracklab/fmriprep:20.0.4
RUNNING: docker run --rm -it -e DOCKER_VERSION_8395080871=19.03.8 -v /Users/frederic/code/niworkflows_jdkent/niworkflows:/usr/local/miniconda/lib/python3.7/site-packages/niworkflows:ro -v /Applications/freesurfer/license.txt:/opt/freesurfer/license.txt:ro -v /Users/frederic/Dropbox_PHC/MR_data/ucb_data/prepostgad:/data:ro -v /Users/frederic/Dropbox_PHC/MR_data/ucb_data/prepostgad/derivatives:/out -v /Users/frederic/Documents/MR_data/workdirs/prepostgad:/scratch poldracklab/fmriprep:20.0.4 /data /out participant --participant_label=B13293 --mem_mb=24000 --nthreads=8 --omp-nthreads=8 --use-syn-sdc --use-aroma --fs-no-reconall -w /scratch --output-spaces MNI152NLin6Asym:res-2 anat func:res-native
Traceback (most recent call last):
  File "/usr/local/miniconda/bin/fmriprep", line 10, in <module>
    sys.exit(main())
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 296, in main
    opts = get_parser().parse_args()
  File "/usr/local/miniconda/lib/python3.7/site-packages/fmriprep/cli/run.py", line 42, in get_parser
    from niworkflows.utils.spaces import Reference, SpatialReferences, OutputReferencesAction
ModuleNotFoundError: No module named 'niworkflows.utils.spaces'
fMRIPrep: Please report errors to https://github.com/poldracklab/fmriprep/issues

It does run my version, however, which I updated with your fix (I have no idea why). Unfortunately, that didn't seem to fix things either. I've deleted everything (all work directories and derivative directories) and I'm trying again to see if maybe there was something cached that was causing the issue...

@jdkent
Copy link
Collaborator

jdkent commented Mar 18, 2020

sorry about the error, it may be because the repo /Users/frederic/code/niworkflows_jdkent/ is from my master branch instead of the maint/1.1.x_fix_ceagent branch.

To check this you can type the following:

cd /Users/frederic/code/niworkflows_jdkent/
git branch

if you get something that looks like * master, then type this:

git fetch origin maint/1.1.x_fix_ceagent:maint/1.1.x_fix_ceagent
git checkout maint/1.1.x_fix_ceagent

then it should work more as expected 🤞

@bbfrederick
Copy link
Contributor Author

Ran clean with my version, no joy. Followed your instructions, switched to your version, deleted the figures directory and most of the associated work directories, again, the problem remains. So now I'm deleting everything and running completely clean with your version. I'll report back when done, but I'm not optimistic... There still seems to be some critical piece missing.

@jdkent
Copy link
Collaborator

jdkent commented Mar 19, 2020

just pushed some new commits, I'm reasonably confident those will address your issue. I locally replicated and repaired the issue on my end.

you can get the new commits with:

cd /Users/frederic/code/niworkflows_jdkent/
git pull origin maint/1.1.x_fix_ceagent

Thank you for your patience!

@bbfrederick
Copy link
Contributor Author

That did the trick! Thank you!

In the end, what was the missing bit?

BTW, while you were in there, did you fix it for direction and echo as well? It looked like those were not handled either.

@effigies
Copy link
Member

@bbfrederick If you're still referring to the BIDS_NAME regex, I don't think it's really relevant. (The one place it's used, it's to parse the subject and session, and everything else is ignored anyway.) Please feel free to submit a PR with an updated regular expression, but I don't think it's necessary to hold up @jdkent's PR at this point.

Direction and echo are taken care of in the sections that James was working on that fixed your issue.

@bbfrederick
Copy link
Contributor Author

I agree. I'll submit a separate PR after James' patch is applied just to satisfy my OCD.

@effigies
Copy link
Member

PR merged on the niworkflows side. If you want this patch in the next fMRIPrep release, could you submit ASAP? Base your branch off of the maint/1.1.x branch.

If you don't care about it being in the next release, let me know and I'll start the release process.

@bbfrederick
Copy link
Contributor Author

I assume that was directed at @jdkent

@effigies
Copy link
Member

@bbfrederick Nope that was directed at you. @jdkent's patch will be in this release.

@jdkent
Copy link
Collaborator

jdkent commented Mar 19, 2020

@bbfrederick The missing bit was @effigies suggestion above, I didn't include it earlier since I didn't know where it was being used until recently.
The relevent bit is here.

The linked code above looks in the reportlets directory and if it finds an svg file, it generates a "bids" valid figure name based on figures.json, and copies the file over. The layout.build_path method is accepting of any key-value pair (i.e., will not raise an error), but it will ignore key-value pairs that are not a part of the default_build_patterns in figures.json, resulting in the bug you identified where the ce- key was being ignored which led to build_path creating the same filename (e.g., instead of sub-01_task-rest_ce-Gd_bold.svg build_path returned sub-01_task-rest_bold.svg) repeatedly and copying the different files in the reportlets directory to the same filename in the figures directory, overwriting whatever file was there before. For example,

/work/reportlets/.../sub-01_task-rest_ce-Gd_bold.svg -> /fmriprep/.../figures/sub-01_task-rest_bold.svg
/work/reportlets/.../sub-01_task-rest_ce-none_bold.svg -> /fmriprep/.../figures/sub-01_task-rest_bold.svg # The file got overwritten!

Hope that makes sense!

@effigies I actually see some other items to clean in figures.json and I will open a pull request to deal with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants