Split output files by heterogeneous offset#1752
Conversation
57f7017 to
ca34de4
Compare
h-mayorquin
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Before we settle on a mechanism I'd like to understand what angle you're coming from, because there may already be a path for part of this. In our earlier European Data Format (EDF) conversions we've used channels_to_skip together with a ConverterPipe to carve a single file into channel groups and write more than one series from it, so a single file fans out into multiple ElectricalSeries (with auxiliary channels grouped by unit). That pattern is documented here: Combining Electrode and Auxiliary Channels. So there's a path today.
That said, I do think it's worth making this more automatic, which is what I take you to be going for here. Ideally, we want to automatically create different ElectricalSeries for each signal kind (e.g. [fill here]). My worry is that the channel offset isn't a good key to partition the ElectricalSeries on automatically. The offset is a derived value (it falls out of each channel's physical min/max), so it doesn't track signal kind: different kinds can land on the same offset, and channels of the same kind can land on different offsets. Partitioning on it therefore both over-splits coherent montages and can lump unrelated channels together. Concretely, in the file that originally motivated #1106 the differing offsets all come from non-neural channels (the DC channels and a plethysmography channel) that happen to be labeled in microvolts, while the 256 actual electrode channels share a single offset, so there channels_to_skip is really all that's needed.
The way I'd like to improve this is upstream, in python-neo (the library we use to read these files), by making the separation of signals by kind smarter, so the grouping is meaningful instead of keyed on a symptom. Separately, I did find that some files genuinely have the same signal kind with different per-channel offsets (some per-channel-autoscaling exporters do this). I think the NWB schema should support that and I'll follow up there, but in the meantime I agree we should offer an escape hatch to write that edge case without rescaling the data, even if the result is slightly sub-optimal.
So the thing that would help me most: what kind of data are you coming from? Do you genuinely have the same signal kind with different offsets in one file, or are the differing offsets coming from different kinds of channels bundled together? This would be useful to set a course.
…cordingExtractorInterface.run_conversion_split_by_offset` to support recordings (e.g. some EDF files) whose channels have heterogeneous offsets, which cannot be represented in a single NWB `ElectricalSeries`. The channels are partitioned by offset value and each partition is written to its own NWB file (paths derived by inserting an `_offset{index}` suffix). Recordings with a single offset are written to one file unchanged.
|
Thanks for the feedback. I am particularly interested in files with the same kind of data with different offsets, such as both extra-cranial and intra-cranial EEG signals. Considering the "Combining Electrode and Auxiliary Channels" path that already exists, I will scale back this PR to just the split_by_offset() function to identify heterogeneous offsets so they can be handled using that path. |
Rework the heterogeneous-offset support to a single public method, `BaseRecordingExtractorInterface.split_by_offset`, which partitions a recording's channels by offset value (using only the offsets present in the recording, independent of any external/BIDS metadata) and returns one sub-interface per distinct offset, each with a distinct `es_key`. The sub-interfaces can be combined in a `ConverterPipe` to write one `ElectricalSeries` per offset into a single NWB file, or converted individually. - `add_to_nwbfile` is unchanged: a heterogeneous-offset recording still raises the informative error (no automatic splitting, no new parameter). - Removed `run_conversion_split_by_offset` (multi-file) and the automatic multi-series write path from earlier iterations. - Docs and CHANGELOG updated to the split_by_offset + ConverterPipe pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ca34de4 to
be8b1b9
Compare
for more information, see https://pre-commit.ci
|
Thanks Alex. I'd rather not add a new method to
The one tradeoff with (2) is that folding into float gives up the lossless integer round-trip. The fully faithful fix would be to keep the raw ints and store a per-channel offset, which needs a schema change; I've proposed that at NeurodataWithoutBorders/nwb-schema#694, so Would (2) be useful for your case? |
Related to #1106
Added BaseRecordingExtractorInterface.split_by_offset and BaseRecordingExtractorInterface.run_conversion_split_by_offset to support recordings (e.g. some EDF files) whose channels have heterogeneous offsets, which cannot be represented in a single NWB ElectricalSeries. The channels are partitioned by offset value and each partition is written to its own NWB file (paths derived by inserting an _offset{index} suffix). Recordings with a single offset are written to one file unchanged.