Derive AcquisitionMetadata.n_channels from channel_types instead of storing it - #1166
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d61a622314
ℹ️ 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".
| @property | ||
| def n_channels(self) -> int: |
There was a problem hiding this comment.
Remove n_channels from the remaining coverage fixture
Running pytest -q moabb/tests/test_bids_enrichment.py::TestReadmeAcquisitionFields now produces 17 failures because test_field_present still includes n_channels in the constructor kwargs at moabb/tests/test_bids_enrichment.py:2164. Since this property replaces the constructor field, update that parametrized coverage test to derive the count through channel_types (and remove or adapt its n_channels case) so the normal test suite can pass.
Useful? React with 👍 / 👎.
AcquisitionMetadata stored `n_channels` next to `channel_types`, which
describes the same fact. The two could disagree, and for 34 of the 147
catalogued datasets they did.
The cause was not carelessness: the field was being used with two different
meanings. Dreyer2023A stored 27, counting only its EEG electrodes, while its
own channel_types ({eeg: 27, emg: 2, eog: 3}) and its own 32-entry sensors
list both describe 32 channels. 48 datasets declare more than one channel
type, so the two readings diverge for a third of the catalogue.
Rather than add a validator for a field that should not exist, the field is
gone and `n_channels` is a property over `channel_types`. It now consistently
means every recorded channel -- the reading that `sensors` and
`test_n_channels_matches_raw_data` (which counts every non-stim channel)
already assumed. The 34 disagreements are not fixed so much as made
impossible, and `validate_metadata_against_dataset` needs no new assertion.
218 `n_channels=` arguments come out across 93 files, together with the
enrichment branch in metadata/__init__.py whose only job was to backfill the
value. One test asserted the old EEG-only reading for Dreyer2023A and now
asserts the total.
Net -265 lines. Split out of NeuroTechXgh-1161 so the mechanical change is reviewable
on its own; closes NeuroTechXgh-1163.
d61a622 to
d4f3eee
Compare
…nnels
The four GuttmannFlury2025 classes declared `"stim": 1` in `channel_types`.
That was harmless while `n_channels` was stored independently, but now that it
is `sum(channel_types.values())` it makes the four report 66 recorded channels
where 65 were recorded -- MOABB adds the stim channel itself, as
`test_metadata_matches_raw_data` says in the comment above the
`raw_counts.pop("stim", None)` it does before comparing.
That test is `@pytest.mark.download`, and NeuroTechXgh-1161 is the change that repairs
the monthly download job (it pointed at a file that does not exist and
collected nothing), so merging both would have surfaced this as an off-by-one
on four datasets.
…ive-n-channels # Conflicts: # moabb/datasets/bnci/bnci_2025.py
Split out of #1161 so the mechanical part is reviewable on its own. Net −265 lines.
The problem
AcquisitionMetadatastoredn_channelsnext tochannel_types, which describes the same fact. The two could disagree, and for 34 of the 147 catalogued datasets they did.The cause was not carelessness — the field was being used with two different meanings:
48 datasets declare more than one channel type, so the two readings diverge for a third of the catalogue.
The fix
Delete the field; derive it.
It now consistently means every recorded channel — the reading that each dataset's own
sensorslist andtest_n_channels_matches_raw_data(which counts every non-stimchannel) already assumed.The 34 disagreements are not fixed so much as made impossible, which is why this adds no validator and no invariant test.
validate_metadata_against_datasetneeds no new assertion.What that removes
n_channels=arguments across 93 filesmetadata/__init__.pywhose only job was to backfill the valuereplace(...)defaults that also disagreed with their ownchannel_typesBehaviour change
For the 34 datasets that stored the EEG-only count,
n_channelsnow reports the total.Dreyer2023Agoes 27 → 32. One test asserted the old reading and now asserts the total; it is the only one in the suite that pinned a literal value for a multi-type dataset.Documented under API changes, not Bugs: passing
n_channels=to the constructor is no longer accepted.Verification
n_channels != sum(channel_types): 34 → 0, and now unrepresentableAcquisitionMetadata(...)call site still passessampling_rateandchannel_types— checked by AST across the package, 0 brokentest_lee2024_data_path_downloads_the_real_upstream_inventory, is pre-existing ondevelop)pre-commitcleanCloses #1163.