Add channel rereferencing across language bindings - #847
Conversation
Andrey1994
left a comment
There was a problem hiding this comment.
Thanks for implementing this across all supported bindings. The core algorithm looks correct, in particular computing the complete reference signal before mutating the target channels, which makes overlapping target/reference lists safe.
There are a few things I think we should address before taking this out of draft:
-
Duplicate channel indices currently have inconsistent semantics across bindings. Most implementations subtract the reference repeatedly when
channels_to_referencecontains the same channel more than once, while MATLAB's indexed assignment appears to update that row only once. Duplicates inreference_channelsalso implicitly give that channel additional weight. Please either reject duplicates in both lists or explicitly define and test identical behavior in every binding. I would prefer rejecting them. -
Regression coverage is too limited for nine independent implementations. At present, dedicated tests are added only for Julia, Rust, and Swift; the C++ implementation itself is not covered. Please add authoritative C++ coverage for the calculation, overlapping lists, empty/out-of-range arguments, and the chosen duplicate semantics. The independently implemented bindings should also have at least smoke/parity coverage—the modified downsampling examples do not verify the result.
-
The branch needs to be updated and CI rerun. GitHub currently reports the PR as non-mergeable, and the Valgrind and CppCheck runs were cancelled. We should have an up-to-date, complete CI result before marking it ready.
Non-blocking: I would prefer a separate rereferencing example instead of changing every downsampling example. Combining unrelated operations makes those examples less focused. It would also be useful to document explicitly that MATLAB returns a referenced copy while the other APIs mutate their input.
The overall API direction matches #619, and the overlap handling itself looks good.
|
Update: the workflow hardening has been moved into a separate draft PR built directly on the current The two cancelled checks are infrastructure failures, not failures in the rereferencing code:
Both cancelled jobs have been re-run (attempt 3) and are currently queued. PR #857 addresses recurrence by using the official fallback mirrors, bounding package-install time, consolidating Valgrind's APT setup, and updating the Node-based actions to v7. |
Summary
referenceoperation to the C++, C#, Java, Julia, MATLAB, Node.js, Python, Rust, and Swift DataFilter APIsMotivation
EEG pipelines commonly need to re-reference multiple channels against the mean of one or more reference electrodes. BrainFlow currently requires users to implement that operation themselves in every language. This adds one consistent operation following the API proposed in #619.
For input matrix
data, the implementation first computesreference_signal[t] = mean(data[channel, t] for channel in reference_channels)and then applies
data[channel, t] -= reference_signal[t]to every channel in
channels_to_reference. The reference signal is computed completely before any target row is changed.Closes #619.
Validation
rtol=1e-13; invalid lists and indices were also checkednpm run buildpassed; direct runtime behavior and invalid-argument checks passedbrainflow.slnRelease build passed with 0 warnings and 0 errorsswift-downsamplingproduct both build successfullygit diff --checkpassedThe draft PR lets the full BrainFlow CI validate the remaining platform-specific C++, Java, Julia, MATLAB, R, Rust, and Swift test paths.