Repository navigation
Add Python DataFilter.rereference_channels for EEG/EXG - #870
OgarkovAnton wants to merge 5 commits into
Conversation
Implements brainflow-dev#619 with a Python-only helper that subtracts the mean of selected reference channels from non-excluded channels, plus unit tests and CI coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
Andrey1994
left a comment
Andrey1994
left a comment
There was a problem hiding this comment.
hi and thanks!
https://brainflow.readthedocs.io/en/stable/BrainFlowDev.html should be a good starting point about how c++ methods are called from other languages in brainflow, this should follow the same pattern instead of implementing it in python directly
| :type excluded_channels: list or tuple or ndarray or None | ||
| :return: new 2d array with the same shape as ``data`` | ||
| :rtype: NDArray[Shape["*, *"], Float64] | ||
| :raises ValueError: if ``data`` is not a non-empty 2d array, ``reference`` is empty, |
There was a problem hiding this comment.
instead of implementing it in the python layer directly it should follow the workflow as for other methods(should be done in c++ and other languages should call it from there)
Move channel re-referencing from a Python-only helper into the native DataHandler C API with an in-place channels/reference_channels API, then expose it through C++ and the other language bindings per review on brainflow-dev#619. Co-authored-by: Cursor <cursoragent@cursor.com>
Andrey1994
left a comment
Andrey1994
left a comment
There was a problem hiding this comment.
Reviewed f1957c1.
The earlier architectural feedback is addressed: rereferencing now lives in native C++, with wrappers across all nine language bindings. There are still correctness issues in Python channel-index handling and a Node/native validation mismatch that should be fixed before merging. Inline comments include reproductions and suggested fixes, plus smaller Java validation and documentation corrections.
Validation: the native/C++ build and signal-processing regression passed; all 10 new Python rereferencing tests and 17 existing binding-validation tests passed; TypeScript compiled. Additional runtime checks reproduced the two Python issues and the Node discrepancy below. Please add regression coverage for these cases, since the existing tests pass despite them.
| if channels_arr.ndim != 1 or reference_arr.ndim != 1: | ||
| raise BrainFlowError('channel index arrays must be 1d', | ||
| BrainFlowExitCodes.INVALID_ARGUMENTS_ERROR.value) | ||
| res = DataHandlerDLL.get_instance().rereference_channels( |
There was a problem hiding this comment.
[P2] Make both channel-index buffers contiguous before the native call
numpy.asarray(..., dtype=numpy.int32) preserves strides when its input is already an int32 array, while the native function reads contiguous int* buffers. Reproduced with channels = np.arange(4, dtype=np.int32)[::2]: the caller selects [0, 2], but rereference_channels(data, channels, [3]) modifies rows 0 and 1. Using the same sliced array as reference_channels also computes the wrong reference. Normalize both arrays with numpy.ascontiguousarray after validating them, and enforce contiguous channel buffers in the ctypes declarations. Please test sliced target and reference arrays.
| if data.dtype != numpy.float64 or not data.flags.writeable: | ||
| raise BrainFlowError('data must be a writable float64 array', | ||
| BrainFlowExitCodes.INVALID_ARGUMENTS_ERROR.value) | ||
| channels_arr = numpy.asarray(channels, dtype=numpy.int32) |
There was a problem hiding this comment.
[P2] Validate channel types and bounds before narrowing to int32
Casting first destroys the information needed to reject invalid indices. Reproduced: [0.9] and [-0.5] both become row 0; np.array([4294967296], dtype=np.int64) wraps to row 0 as well. All three calls succeed and modify data instead of raising INVALID_ARGUMENTS_ERROR. Validate that both selections contain integers within the original array's row bounds before converting them to contiguous int32 buffers. Add regression tests for fractional, negative fractional, and overflowing integer indices.
| public static rereferenceChannels( | ||
| data: number[][], channels: number[], referenceChannels: number[]): void | ||
| { | ||
| const columns = DataFilter.checkMatrix(data, true); |
There was a problem hiding this comment.
[P2] Restrict finiteness validation to target and reference channels
checkMatrix(data, true) rejects NaNs in every row, including rows that are neither updated nor used as references. For example, data = [[1, 2], [3, 4], [NaN, NaN]], channels = [0], and referenceChannels = [1] throws here, whereas the native implementation and Python correctly produce [-2, -2] in row 0 and preserve the unrelated NaN row. Keep the matrix shape checks, but only require finite values in the selected target/reference rows, as the two checkChannels calls already do. Add a test with a non-finite unselected channel.
| throw new BrainFlowError ("invalid args for rereference_channels", | ||
| BrainFlowExitCode.INVALID_ARGUMENTS_ERROR.get_code ()); | ||
| } | ||
| int cols = data[0].length; |
There was a problem hiding this comment.
[P3] Check the first row for null before reading its length
data[0].length is evaluated before the loop's null-row validation. An input whose first row is null therefore throws NullPointerException, while null rows later in the matrix produce the intended BrainFlowError. Check data[0] before this dereference so malformed matrices consistently return INVALID_ARGUMENTS_ERROR.
|
|
||
| ``DataFilter.rereference_channels`` subtracts the element-wise mean of | ||
| ``reference_channels`` from the selected ``channels`` of a ``(channels, samples)`` | ||
| array in place. Channel indices are zero-based. Overlap between the two index lists |
There was a problem hiding this comment.
[P3] Document the MATLAB and Julia indexing and mutation conventions
This shared API description says channel indices are zero-based, but the MATLAB and Julia wrappers accept one-based indices and subtract 1 before calling native code. Please make that distinction explicit so users do not select the wrong channels or get errors when following the documentation. MATLAB also returns the updated matrix, so callers need to assign the result rather than relying on the blanket in-place wording.
Implements #619 with a Python-only helper that subtracts the mean of selected reference channels from non-excluded channels, plus unit tests and CI coverage.