Skip to content

Add support for VECTORIZED orientation in BrainVisionRawIO #1797

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for VECTORIZED orientation in BrainVisionRawIO #1797

wants to merge 4 commits into from

Conversation

@Kkuntal990
Copy link

@Kkuntal990 Kkuntal990 commented Oct 17, 2025 •
edited
Loading

Description

Github Issue

This PR adds support for VECTORIZED data orientation in BrainVisionRawIO, which was previously causing a NeoReadWriteError.

Problem

BrainVision files can store data in two orientations:

  • MULTIPLEXED: ch1_s1, ch2_s1, ch3_s1, ch1_s2, ch2_s2, ... (interleaved)
  • VECTORIZED: all ch1 samples, then all ch2 samples, etc. (sequential)

Previously, Neo only supported MULTIPLEXED orientation, causing imports to fail on VECTORIZED files such as those in OpenNeuro dataset ds004621.

Solution

  • Modified the orientation check to accept both MULTIPLEXED and VECTORIZED
  • Implemented custom _get_analogsignal_chunk() method that:
    • Uses the default parent implementation for MULTIPLEXED files (no behavior change)
    • For VECTORIZED files, reads each channel's data from its sequential location in the binary file
  • Maintains full backward compatibility with existing MULTIPLEXED files

Testing

✅ Validation against MNE-Python: Tested on real-world VECTORIZED dataset (ds004621, 127 channels × 740,360 samples)

  • Correlation: 1.0
  • Maximum absolute difference: 0.0
  • Results match MNE-Python exactly

✅ Synthetic tests: Created test files in both orientations and verified correct data reading

✅ Backward compatibility: MULTIPLEXED files continue to work as before

Changes

  • neo/rawio/brainvisionrawio.py:
    • Added self._data_orientation to track orientation type
    • Implemented custom _get_analogsignal_chunk() for VECTORIZED reading
    • Updated error message to reflect support for both orientations

Related Issue

@Kkuntal990 @claude
Add support for VECTORIZED orientation in BrainVisionRawIO
35d4a41 
Fixes KAN-43: https://eeglab.atlassian.net/browse/KAN-43

Previously, BrainVisionRawIO only supported MULTIPLEXED data orientation
(interleaved channels), causing a NeoReadWriteError when attempting to
read files with VECTORIZED orientation (sequential channel data).

Changes:
- Modified data orientation check to accept both MULTIPLEXED and VECTORIZED
- Added custom _get_analogsignal_chunk() method to handle VECTORIZED reading
- For VECTORIZED files, reads each channel's data from its sequential
  location in the binary file
- Maintains backward compatibility with MULTIPLEXED files (uses parent
  class implementation)

Testing:
- Validated against MNE-Python on real-world VECTORIZED dataset (ds004621)
  with 127 channels × 740,360 samples - results match exactly (correlation=1.0)
- Tested both MULTIPLEXED and VECTORIZED orientations with synthetic data
- All existing functionality preserved for MULTIPLEXED files

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@zm711
Copy link
Contributor

zm711 commented Oct 19, 2025

Thanks @Kkuntal990. We will try to review this soon. I've run the tests now.

@h-mayorquin
Copy link
Contributor

h-mayorquin commented Oct 20, 2025

Can you open an issue here in neo? I can't see the jira board.

@Kkuntal990
Copy link
Author

Kkuntal990 commented Oct 22, 2025

Can you open an issue here in neo? I can't see the jira board.

Done

@samuelgarcia
Copy link
Contributor

samuelgarcia commented Oct 23, 2025

Thanks a lot for adding the other data layout in brainvision.

2 points:

  1. I am not sure to like the np.fromfile() implementation maybe we could also use memmap here.
    In multiprocessing onlinux with mp_context=fork this lead to errors because processes share the same fid.

  2. Also,maybe, we could move this logic extend in base the BaseRawIO._get_analogsignal_chunk() and we should add a time_axis=0/1 concept to describe the data layout (time, chans) or (chans, times).

@Kkuntal990
Simplify VECTORIZED implementation to use memmap
9fc58a0 
- Replace np.fromfile() with np.memmap for multiprocessing compatibility
- Minimize code changes in _get_analogsignal_chunk()
- Remove file handle caching logic (memmap handles this)
- All tests still pass with identical results
@Kkuntal990
Refactor VECTORIZED support using time_axis in BaseRawIO
d214b4a 
Moved VECTORIZED orientation logic to BaseRawIO as suggested by @samuelgarcia:
- Added time_axis parameter to buffer_description (0=MULTIPLEXED, 1=VECTORIZED)
- Extended BaseRawIO._get_analogsignal_chunk() to handle time_axis=1 for raw buffers
- Removed custom _get_analogsignal_chunk() override from BrainVisionRawIO
- Fixed _get_signal_size() to correctly handle raw buffers with time_axis=1

Benefits:
- Cleaner, more general solution applicable to other readers
- Consistent with existing HDF5 time_axis pattern
- Reduced code duplication
- All tests pass with identical MNE-Python validation
@Kkuntal990
Fix shape convention to be consistent with HDF5 pattern
7bf5208 
When time_axis=1, shape should be (channels, time) not (time, channels).
This makes raw binary handling consistent with HDF5:

- time_axis=0: shape is (time, channels) - MULTIPLEXED
- time_axis=1: shape is (channels, time) - VECTORIZED

Changes:
- BrainVisionRawIO: Set shape as (channels, time) when VECTORIZED
- BaseRawIO: Use shape[time_axis] consistently for i_stop default
- Removed duplicate time_axis retrieval

All tests pass with identical MNE-Python validation.
@Kkuntal990
Copy link
Author

Kkuntal990 commented Nov 5, 2025 •
edited
Loading

Latest Updates: Refactored to use time_axis pattern in BaseRawIO

Thanks @samuelgarcia for the suggestion! I've refactored the implementation to move the logic into BaseRawIO using the time_axis parameter, making it consistent with the existing HDF5 pattern.

Changes Made

1. Replaced np.fromfile() with np.memmap()

  • Avoids multiprocessing issues with shared file handles
  • More efficient and safer

2. Extended BaseRawIO with time_axis support for raw buffers

  • Added time_axis=1 handling in BaseRawIO._get_analogsignal_chunk()
  • Follows the same convention as HDF5:
    • time_axis=0: shape is (time, channels) - MULTIPLEXED
    • time_axis=1: shape is (channels, time) - VECTORIZED
  • Updated _get_signal_size() to use shape[time_axis] consistently

3. Simplified BrainVisionRawIO

  • Sets time_axis in buffer_description based on DataOrientation
  • Adjusts shape to match time_axis convention (for VECTORIZED: (channels, time))
  • No custom override needed - relies entirely on BaseRawIO

4. Fixed shape convention consistency

  • Initially had inconsistency where raw buffers used (time, channels) even with time_axis=1

  • Now fully consistent: time_axis=1 means shape is (channels, time) for both raw and HDF5

  • More general: Other readers can now use time_axis for similar layouts

  • Cleaner: Removed BrainVision-specific override logic

  • Consistent: Same pattern throughout BaseRawIO (raw & HDF5)

Please review when you can

@Kkuntal990
Copy link
Author

Kkuntal990 commented Nov 11, 2025

@samuelgarcia @h-mayorquin if possible could you please review the PR ?

Thank you for your time.

@samuelgarcia
Copy link
Contributor

samuelgarcia commented Nov 14, 2025

Hi @Kkuntal990.
Really sorry for the dealy.
I will try my best next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Kkuntal990 @zm711 @h-mayorquin @samuelgarcia