From ed96c83c6a26e6be4ff34a105319bedbb0106fcd Mon Sep 17 00:00:00 2001 From: peerherholz Date: Tue, 9 Jun 2026 09:15:45 +0200 Subject: [PATCH 1/4] test(cli): events.tsv resolution for concat-runs analysis report --- ffrprep/tests/test_cli.py | 74 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/ffrprep/tests/test_cli.py b/ffrprep/tests/test_cli.py index 96e311f..5d6521f 100644 --- a/ffrprep/tests/test_cli.py +++ b/ffrprep/tests/test_cli.py @@ -4,6 +4,7 @@ work the same on free functions as on methods. """ import argparse +import json import os import tempfile from pathlib import Path @@ -949,6 +950,79 @@ def test_collect_evoked_groups_empty_dir_returns_empty_list(tmp_path): assert _collect_evoked_groups(a_dir) == [] +# --------------------------------------------------------------------------- +# _resolve_events_fpath_for_group: events.tsv resolution per (task, run) +# group, including the concat-runs case where the evoked filename +# carries no `run-*` token and the sidecar's ConcatenatedRuns list +# is the source of truth. +# --------------------------------------------------------------------------- + + +def test_resolve_events_fpath_for_group_concat_runs_uses_first_listed_run( + tmp_path, +): + """Concat-runs: the evoked filename has no run token, so the + sidecar's ``ConcatenatedRuns`` list must drive events.tsv + resolution. Without this, the stim-correlation helpers silently + no-op because ``sub-XX_task-YY_events.tsv`` (no run) does not + exist in a per-run BIDS dataset. + """ + from ffrprep.ffrprep_cli import _resolve_events_fpath_for_group + + bids_root = tmp_path / "ds" + (bids_root / "sub-01" / "eeg").mkdir(parents=True) + + analysis_dir = tmp_path / "deriv" / "sub-01" + evoked_fpath = _touch_evoked( + analysis_dir, "sub-01_task-active_desc-evoked.fif" + ) + evoked_fpath.with_suffix(".json").write_text( + json.dumps({"ConcatenatedRuns": ["1", "2"]}) + ) + + grp = { + "task": "active", + "run": None, + "evoked_files": [evoked_fpath], + } + resolved = _resolve_events_fpath_for_group(grp, "01", bids_root) + expected = ( + bids_root / "sub-01" / "eeg" + / "sub-01_task-active_run-1_events.tsv" + ) + assert resolved == expected + + +def test_resolve_events_fpath_for_group_single_run_passes_through(tmp_path): + """Single-run: ``grp['run']`` is the literal run token, no sidecar + fallback needed. Regression guard so the concat-runs fix doesn't + break the existing per-run path. + """ + from ffrprep.ffrprep_cli import _resolve_events_fpath_for_group + + bids_root = tmp_path / "ds" + (bids_root / "sub-01" / "eeg").mkdir(parents=True) + + analysis_dir = tmp_path / "deriv" / "sub-01" + evoked_fpath = _touch_evoked( + analysis_dir, "sub-01_task-active_run-1_desc-evoked.fif" + ) + # No sidecar — exercises the empty-meta fallback path so the + # function never depends on the sidecar in the single-run case. + + grp = { + "task": "active", + "run": "1", + "evoked_files": [evoked_fpath], + } + resolved = _resolve_events_fpath_for_group(grp, "01", bids_root) + expected = ( + bids_root / "sub-01" / "eeg" + / "sub-01_task-active_run-1_events.tsv" + ) + assert resolved == expected + + # --------------------------------------------------------------------------- # _load_stim_waveform: stimulus loader dispatch # --------------------------------------------------------------------------- From d9d468439eccb35d7474d60231cecaf785ca5846 Mon Sep 17 00:00:00 2001 From: peerherholz Date: Tue, 9 Jun 2026 09:48:27 +0200 Subject: [PATCH 2/4] fix(cli): stim correlation under --concat-runs --- ffrprep/ffrprep_cli.py | 58 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 51 insertions(+), 7 deletions(-) diff --git a/ffrprep/ffrprep_cli.py b/ffrprep/ffrprep_cli.py index f562506..39d6e0e 100644 --- a/ffrprep/ffrprep_cli.py +++ b/ffrprep/ffrprep_cli.py @@ -382,6 +382,55 @@ def _collect_evoked_groups(analysis_dir): return list(groups_map.values()) +def _resolve_events_fpath_for_group(grp, subject, bids_root): + """Resolve the events.tsv path for one analysis group. + + Concat-runs analysis outputs carry no ``run-*`` token in their + filenames, so ``grp['run']`` is ``None`` and a literal + ``sub-XX_task-YY_events.tsv`` lookup misses (the source BIDS + dataset only has per-run events files). Read the sidecar's + ``ConcatenatedRuns`` list and use the first listed run; the + ``stim_file`` column is invariant across runs for a given + ``trial_type`` so any run is sufficient. Falls back to + ``grp['run']`` when no ``ConcatenatedRuns`` field is present + (the single-run case). + + Parameters + ---------- + grp : dict + One entry from :func:`_collect_evoked_groups`. Must carry + ``task``, ``run`` and ``evoked_files`` keys. + subject : str + Subject label (without the ``sub-`` prefix). + bids_root : pathlib.Path or str + BIDS dataset root. + + Returns + ------- + pathlib.Path + The resolved events.tsv path. May not exist on disk if the + source dataset doesn't carry events files; downstream + callers handle the missing-file case as a silent no-op. + """ + import json + + first_sidecar = grp["evoked_files"][0].with_suffix(".json") + sidecar_meta = ( + json.loads(first_sidecar.read_text()) + if first_sidecar.exists() else {} + ) + runs = sidecar_meta.get("ConcatenatedRuns") or ( + [grp["run"]] if grp["run"] is not None else [] + ) + first_run = next((r for r in runs if r is not None), None) + eeg_dir = Path(bids_root) / f"sub-{subject}" / "eeg" + if first_run is not None: + return eeg_dir / ( + f"sub-{subject}_task-{grp['task']}_run-{first_run}_events.tsv" + ) + return eeg_dir / f"sub-{subject}_task-{grp['task']}_events.tsv" + + def _make_analysis_payload(args_snap, deriv_snap, subject, group): """Assemble the input dict for one per-(task, run) analysis worker. @@ -1134,13 +1183,8 @@ def _build_analysis_report(args, derivatives_info, subject): task = grp["task"] run = grp["run"] sections = [] - events_fpath = ( - bids_root / f"sub-{subject}" / "eeg" - / ( - f"sub-{subject}_task-{task}_run-{run}_events.tsv" - if run is not None - else f"sub-{subject}_task-{task}_events.tsv" - ) + events_fpath = _resolve_events_fpath_for_group( + grp, subject, bids_root, ) for idx, evo_fpath in enumerate(grp["evoked_files"]): all_files.append(evo_fpath) From 0946e2da7ab8d61d305ed9ea33f96b5d8b535b40 Mon Sep 17 00:00:00 2001 From: peerherholz Date: Wed, 10 Jun 2026 08:46:25 +0200 Subject: [PATCH 3/4] test(cli): _concat_payload_runs filters None placeholders --- ffrprep/tests/test_cli.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/ffrprep/tests/test_cli.py b/ffrprep/tests/test_cli.py index 5d6521f..892ca57 100644 --- a/ffrprep/tests/test_cli.py +++ b/ffrprep/tests/test_cli.py @@ -993,6 +993,36 @@ def test_resolve_events_fpath_for_group_concat_runs_uses_first_listed_run( assert resolved == expected +def test_concat_payload_runs_passes_discovered_list_through(tmp_path): + """Auto-discovered runs (no --run) become the concat payload's + run_label so ``save_preprocessing_outputs`` writes + ``ConcatenatedRuns`` to the preproc sidecar — without which the + analysis-report builder can't resolve the source events.tsv. + """ + from ffrprep.ffrprep_cli import _concat_payload_runs + + assert _concat_payload_runs(["1", "2", "3"]) == ["1", "2", "3"] + + +def test_concat_payload_runs_filters_none_placeholder(tmp_path): + """A dataset with no run-token surfaces as ``[None]`` from + ``get_sessions_tasks_runs``. That sentinel must be dropped so + the saver doesn't write ``ConcatenatedRuns: ['None']``. + """ + from ffrprep.ffrprep_cli import _concat_payload_runs + + assert _concat_payload_runs([None]) is None + assert _concat_payload_runs([]) is None + assert _concat_payload_runs(None) is None + + +def test_concat_payload_runs_mixed_keeps_real_drops_none(tmp_path): + """Mixed lists: keep the real run IDs, drop the None sentinels.""" + from ffrprep.ffrprep_cli import _concat_payload_runs + + assert _concat_payload_runs(["1", None, "2"]) == ["1", "2"] + + def test_resolve_events_fpath_for_group_single_run_passes_through(tmp_path): """Single-run: ``grp['run']`` is the literal run token, no sidecar fallback needed. Regression guard so the concat-runs fix doesn't From 5e5f3d4862c8983c37bcdf90bfd32a0521fcc468 Mon Sep 17 00:00:00 2001 From: peerherholz Date: Wed, 10 Jun 2026 08:46:43 +0200 Subject: [PATCH 4/4] fix(cli): write ConcatenatedRuns under --concat-runs without --run --- ffrprep/ffrprep_cli.py | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/ffrprep/ffrprep_cli.py b/ffrprep/ffrprep_cli.py index 39d6e0e..b9bd3db 100644 --- a/ffrprep/ffrprep_cli.py +++ b/ffrprep/ffrprep_cli.py @@ -238,6 +238,39 @@ def _make_preproc_payload(args_snap, deriv_snap, subject, task_label, run_label, } +def _concat_payload_runs(runs): + """Run list for a concat-runs worker. + + Returns the list when at least one real run ID is present, else + ``None``. Filtering out ``None`` placeholders matters because + ``get_sessions_tasks_runs`` returns ``[None]`` for datasets that + don't use a ``run-*`` token at all; passing that straight through + would write ``ConcatenatedRuns: ['None']`` to the sidecar, which + in turn breaks the analysis-report builder's events.tsv path + resolution. + + Always returning a list (not ``None``) when real runs exist is + what lets ``save_preprocessing_outputs`` populate + ``ConcatenatedRuns`` on the preproc sidecar — required so the + analysis report can pick a source events.tsv to read the + ``stim_file`` column from. + + Parameters + ---------- + runs : list or None + The run list collected upstream (either the user's + ``--run`` selection or the dataset's auto-discovered list). + May contain ``None`` sentinels for no-run-token datasets. + + Returns + ------- + list[str] or None + The cleaned run list, or ``None`` when no real runs remain. + """ + real = [r for r in (runs or []) if r is not None] + return real if real else None + + def _make_concat_payload(args_snap, deriv_snap, subject, task_label, runs, ref_channels, effective_l, effective_h, baseline, reject_value): @@ -2080,7 +2113,7 @@ def run_ffrprep(): payloads = [ _make_concat_payload( args_snap, deriv_snap, subject, task_label, - runs if args.run else None, + _concat_payload_runs(runs), ref_channels, effective_l, effective_h, baseline, reject_value, )