mne-tools / mne-bids-pipeline

Automatically process entire electrophysiological datasets using MNE-Python.
https://mne.tools/mne-bids-pipeline/
BSD 3-Clause "New" or "Revised" License
143 stars 68 forks source link

Allow separate MRIs per session (for longitudinal studies) #987

Closed drammock closed 2 months ago

drammock commented 3 months ago

WIP. Could use some advice about the best approach here; so far it should work (but yet untested!) for my use case but definitely won't work for the "normal" case that is currently supported on main. See the (small) diff for a prominent comment explaining the issue.

Before merging …

drammock commented 2 months ago

eh, this is not going to work as-is. There is an assumption baked-in in too many places that there's only a single folder level between subjects_dir and the various freesurfer files. I'll explore making the folders something like sub-116_ses-a/ rather than sub-116/ses-a/

larsoner commented 2 months ago

Makes sense

drammock commented 2 months ago

I think this is ready for review. Probably needs some addition to the docs so that folks other than me know how to actually make this work... I can add that along with whatever other changes y'all want.

larsoner commented 2 months ago

I think next you'd want to add docs/source/v1.9.md.inc to say that per-session MRIs are now supported.

Any idea if there is a dataset on openneuro that we could add to the test suite that would hit the new code path?

drammock commented 2 months ago

I think next you'd want to add docs/source/v1.9.md.inc to say that per-session MRIs are now supported.

Done. As for adding more thorough documentation elsewhere, I can't actually see where I would add it (since it doesn't involve a config value, and there's no dataset to add that could serve as an example).

Any idea if there is a dataset on openneuro that we could add to the test suite that would hit the new code path?

I doubt it; OpenNeuro has zero datasets with modality=MEG and study_type=Longitudinal :(

drammock commented 2 months ago

Just checked the EEG Longitudinal studies too; none of them involve source reconstruction, only 1 even includes MRI at all, and it's only at 1 timepoint (https://openneuro.org/datasets/ds004024/versions/1.0.1)

larsoner commented 2 months ago

@hoechenberger any idea about the error:

    raise RuntimeError(
RuntimeError: Error 404 when trying to download /home/circleci/mne_data/ds000248/derivatives/freesurfer/subjects/fsaverage/label/Yeo_Brainmap_fsaverage_README from https://s3.amazonaws.com/openneuro.org/ds000248/derivatives/freesurfer/subjects/fsaverage/label/Yeo_Brainmap_fsaverage_README?versionId=WuMze1X4S_cLoRCbEs5IpCIOJMe2VT41

Do we need to raise a bug for the openneuro folks again?

hoechenberger commented 2 months ago

yes seems like it! do you want to take a look? just follow what i did in my previous bug reports :)

drammock commented 2 months ago

FWIW the CIs were all green on the commit prior to the changelog entry, so it's probably safe to merge this (though def. should raise the download issue with the openneuro folks)

larsoner commented 2 months ago

https://github.com/OpenNeuroOrg/openneuro/issues/3145 and https://github.com/hoechenberger/openneuro-py/pull/187, openneuro folks usually resolve quickly so I say let's wait a day or two then restart. If they don't respond by then let's just merge.

hoechenberger commented 2 months ago

Issue has already been addressed upstream by OpenNeuro, restarting CI!

hoechenberger commented 2 months ago

@larsoner This is green. i won't have time to review, please move along if you're happy

larsoner commented 2 months ago

Thanks @drammock !