fix ignoring sessions that have no data #1035
Conversation
| @@ -159,9 +172,9 @@ def get_subjects_sessions(config: SimpleNamespace) -> dict[str, list[str] | list | |||
| f"{tuple(sorted(missing_sessions))}, and " | |||
| "`config.allow_missing_sessions` is False" | |||
| ) | |||
| subj_sessions[subject] = sorted(set(cfg_sessions) & set(valid_sessions_subj)) | |||
| if subj_sessions[subject] == []: | |||
| subj_sessions[subject] = [None] | |||
There was a problem hiding this comment.
this wasn't doing what was intended. We want [None] (or (None,)) for datasets where sessions aren't tracked (ie there's just one session). But for datasets where sessions are tracked, we should omit an entry from this subj_sessions dict if there aren't any sessions found for that subj.
mne_bids_pipeline/_config_utils.py
Outdated
| ignore_acquisitions=("calibration", "crosstalk"), | ||
| ignore_suffixes=("scans", "coordsystem"), |
There was a problem hiding this comment.
These two lines are the important bit, for getting MNE-BIDS to only tell us if data files are present.
This ignore_suffixes line requires upstream mne-tools/mne-bids@41ce36a to work, so we'll need to pin MNE-BIDS to that commit, or wait for the next MNE-BIDS release before merging here.
I'm also not really sure if coordsystems.json and scans.tsv are the only possible sidecars that could have the undesirous effect (of having get_entity_vals return a false positive when no data files are actually present for the requested session --- cf. discussion at mne-tools/mne-bids#1351). If you can think of other sidecar suffixes to exclude, speak now.
There was a problem hiding this comment.
so we'll need to pin MNE-BIDS to that commit, or wait for the next MNE-BIDS release before merging here.
This is a tough req since MNE-BIDS does not release super frequently. And in the meantime anyone doing pip install -ve . will have a valid usable (sufficiently up-to-date) editable install overwritten by the pinned install etc.
Since this is a bit of a niche bug (right?), could we instead use inspect to see if the right kwarg is present and pass it if so? Then once MNE-BIDS releases we bump our min req to the one that actually has it and remove the kwarg thing here. If we wait to release MNE-BIDS-Pipeline until MNE-BIDS releases then users are in good shape, and in the meantime devs are also in (sufficiently) good shape. We'd "just" need to make CIs install the dev version of MNE-BIDS, which they might already be doing.
There was a problem hiding this comment.
... i.e., we don't have to wait to merge this PR and we also don't have to explicitly pin below in pyproject.toml
There was a problem hiding this comment.
I can do that, but if someone sets allow_missing_sessions (and actually is missing some sessions) and they don't have a new enough mne_bids, then mne_bids will report that they have data for a given subj+session that they don't actually have data for, and then the pipeline will bomb later.
Maybe I should use inspect here, but also check it during config import, to make sure that if they want to allow_missing_sessions, that they also have a sufficiently capable version of mne_bids?
There was a problem hiding this comment.
Yeah that would work, if they have it set and the kwarg doesn't exist then raise an error, or at least emit a warning.
Should be a transient problem anyway since hopefully we'll release after MNE-BIDS, and then pin in pyproject.toml to >= a released version
drammock
force-pushed
the
fix-allow-missing
branch
from
3b24aa5 to
1b95465
Compare
|
@larsoner @hoechenberger this one is all green and ready for review |
|
well, we know the check works at least: |
|
Accidental TDD is the best kind of TDD |
not when you pay for your CIs 😅 |
|
ok all green again 🙂 |
The addition of
allow_missing_sessionsin #1000 had some problems:[None]in thesubjs_sessionsdict in cases where a subject had no valid sessions; this was not behaving as expectedmne_bids.get_entity_valswas not working as expected, because even if a subject had no data for a session for a given task, the presence ofcoordsystem.jsonorscans.tsvsidecars was causing it to report that the subject had some files for that session.Item (2) was fixed in mne-tools/mne-bids#1362; item (1) is fixed here. Works on my data, but will self-review to add some questions about implementation/testing.
Before merging …
docs/source/dev.md.inc)