Fix the catalogue defects from the full-dataset sweep (#1142, #1143 items 1–3) - #1151
Conversation
Lee2024 (gh-1142), verified against the upstream git tree: subject 8's Doorlock files carry the unpadded id (sub8_*), so a 404 under the padded name retries unpadded; combined and per-block training files coexist in mixtures the configs do not capture, so both forms are requested and a 404 sorts out which exist; param.mat (shipped for all AirConditioner subjects) is requested; and any non-404 failure now raises instead of leaving a silently incomplete directory. RestingStateToP300Adapter (gh-1143 item 3): events=None now means all of the dataset's events, matching how every in-repo usage constructs it, so the adapter can be built with defaults.
…tion The dataset declared all four trigger codes (trajectory_start, waypoint_miss, waypoint_hit, trajectory_end) as trial events under the 90 s trajectory interval. The waypoint and trajectory-end codes are instantaneous point markers, and the loader additionally annotated every non-zero sample of each held trigger pulse, so default paradigm processing tried to epoch 33k+ events of 90 s each per subject (~364 GiB, gh-1143 defect 1). - Declare only trajectory_start as the trial event, matching the ~90 s trajectory interval (32 trials per subject). - Annotate only the onset of each trigger pulse (rising edge / value change) instead of every non-zero sample, so each real event yields a single annotation. Waypoint hit/miss and trajectory-end markers remain available as annotations on the loaded raws. - Add synthetic-raw unit tests covering the declaration, the pulse edge-detection, and the boundedness of default trial derivation.
Kaneshiro2015 declared paradigm="p300" but its six events are object categories (human_body, human_face, animal_body, animal_face, fruit_vegetable, inanimate_object) with no Target/NonTarget, so P300.is_valid/used_events rejected the dataset for every subject. Retag it as paradigm="imagery" (in both the runtime declaration and the experiment metadata) so the n-class paradigms accept it and resolve all six classes, following the existing convention for non-MI multiclass datasets (BNCI2022_001, Shin2017B). Add a regression test asserting MotorImagery/Imagery accept the dataset and resolve its six events, and that P300 does not.
Lee2024: replace the filename-guessing download loop with a real inventory -- the NEMAR deposit's provenance manifest when the sourcedata store is in use (the NEMAR path then never contacts the upstream host), or the upstream git tree otherwise -- and route each file through the shared data_dl, which also serves it from the NEMAR store. Removes the private requests loop, the per-file 404 probing and the padded/unpadded retry storm. BNCI2022-001: one-expression rising-edge detection; trim a comment that restated the class docstring. RestingStateToP300Adapter: fold the events=None default into the existing comprehension. Test files: fold redundant asserts, one-line docstrings, assert against dataset.event_id instead of re-spelled literals.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ce6a84fba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| path=path, | ||
| force_update=force_update, | ||
| verbose=verbose, | ||
| fname=f"{config['dir_name']}/{local}", |
There was a problem hiding this comment.
When loading through the NEMAR provider, _subject_files() returns manifest entries such as Dat_sub01/sub01_Testing1.mat, but this fname prepends Doorlock/ (or another experiment directory). Because data_dl() passes an explicit fname to _store_lookup(), the lookup checks only store/Doorlock/Dat_sub01/... rather than the actual store/Dat_sub01/...; it therefore misses the prefetched file and contacts GitHub. This breaks the guarantee that a provider pinned to "nemar" never accesses upstream and makes loading fail when GitHub is unavailable—the new test does not catch it because data_dl is mocked instead of exercising the store lookup.
Useful? React with 👍 / 👎.
fname names the destination (Lee2024 prefixes the experiment directory and zero-pads subject 8), while the store keeps the upstream names, so probing only fname missed every prefetched file and the NEMAR path fell back to GitHub. Probe fname first, then the URL tails, and cover it with a sockets-forbidden regression test.
|
Good catch on the store lookup — fixed in the latest commit at the |
1 participant
Closes #1142 and fixes items 1–3 of #1143 (item 4, the MartinezCagigal2023Pary subject-16 epoch-length inconsistency, is upstream data and stays open).
BNCI2022_001 — 364 GiB epoching (#1143.1)
Only
trajectory_startis a trial event now (32 bounded, non-overlapping 90 s epochs per subject); the loader annotates each held trigger pulse once (rising edge) instead of once per sample. Verified with the real TU Graz subject-1 file: develop produces exactly 33,114 annotations — the issue's MemoryError shape — vs 1,152 with the fix (32 start / 32 end / 742 hit / 346 miss). Waypoint markers stay on the loader's raws.Kaneshiro2015 — wrong paradigm tag (#1143.2)
Retagged
p300→imagery(the established convention for non-MI multiclass datasets, cf. BNCI2022_001, Shin2017); catalog row andapi.rstgrouping moved with it.MotorImagery()/Imagery()now accept the dataset and resolve all six classes; P300 correctly still rejects it.RestingStateToP300Adapter — unusable defaults (#1143.3)
events=Nonenow means all of the dataset's events (net +0 lines).Lee2024 — 345 silently dropped files (#1142)
data_pathno longer guesses filenames and swallows failures. It downloads a real inventory — the NEMAR deposit's provenance manifest when the sourcedata store is in use (the NEMAR path never contacts the upstream host), or the upstream git tree otherwise — and routes every file through the shareddata_dl, which also serves the bytes from the NEMAR store (S3). Subject 8's unpaddedsub8_*names are normalized to the padded names the loader reads; training-form mixtures andparam.matfall out of the inventory; any failure raises. All claims were verified against the upstream git tree directly, and a full redownload on Expanse now matches upstream file-for-file (only the 5 non-datainfo.mddocs are absent by design).Verification
test_bnci/test_dataset_fixes/test_paradigms/test_metadata; 36 pass on the Lee2024 selection; ruff check/format cleanDeliberate follow-ups (out of scope here)
ERPparadigm so visual-ERP datasets stop borrowing theimagerytag (4th dataset to do so)events=Nonedefault intoBaseParadigm.used_events(currently repeated in 4 paradigms)preserved_annotationsset so point markers surviveSetRawAnnotations