[aaronkanzer]

The BrainVision marker file (.vmrk) counts data points from 1, so a marker at position 1 is on the first sample. BrainVisionRawIO uses the position directly as a 0-based sample index, which puts every event one sample late.

brainvision/File_brainvision_3_int16.vhdr from ephy_testing_data has 294-sample segments at 256 Hz and "New Segment" markers at positions 1, 295 and 589:

from neo.rawio import BrainVisionRawIO
r = BrainVisionRawIO("File_brainvision_3_int16.vhdr"); r.parse_header()
names = list(r.header["event_channels"]["name"])
i = names.index("New Segment")
ts, _, _ = r.get_event_timestamps(event_channel_index=i)
print(ts[:3], r.rescale_event_timestamp(ts[:3], event_channel_index=i))

Observed: [1 295 589] [0.0039 1.1523 2.3008]

Expected: [0 294 588] [0. 1.1484 2.2969], so segment starts land on multiples of the 294-sample segment length. MNE's read_raw_brainvision gives 294 and 588 for the same markers (it subtracts 1 with the comment "BV is 1-indexed").

Fix: subtract 1 from the marker position. Adds a regression test with a small generated .vhdr/.vmrk/.eeg set.

[zm711]

Could you provide a link to documentation explaining this? I didn't directly work on this IO but without a spec I'm not sure how we know if it is 0 or 1 indexed. I don't think we can rely on the fact that another project assuming it is 1-indexed is the right solution.

[aaronkanzer]

Could you provide a link to documentation explaining this? I didn't directly work on this IO but without a spec I'm not sure how we know if it is 0 or 1 indexed. I don't think we can rely on the fact that another project assuming it is 1-indexed is the right solution.

Thanks for the follow-up. I found this via The Brain Products spec. In the spec of of BrainVision Core Data Format 1.0, section 4.3 (page 14), the marker position has valid values >0 and is described as a "1-based" sample position:

https://www.brainproducts.com/download/specification-of-brainvision-core-data-format-1-0/

Happy to add a comment citing the spec next to the change if that helps.

[zm711]

Yeah that would be great. A comment with the link to the spec would be perfect so that no one raises the question in the future. We will have to think about the regression test in general. We prefer to focus on testing on real data as our general testing strategy, but since this was an implementation issue the test might be a good idea.

I'm going to run our tests, but our data repo is having some problems so it is likely the tests will fail. I'll formally review this soon, but I'm working on the testing behind the scenes so we just ask for some patience as we work on getting that fixed!

[aaronkanzer]

Yeah that would be great. A comment with the link to the spec would be perfect so that no one raises the question in the future. We will have to think about the regression test in general. We prefer to focus on testing on real data as our general testing strategy, but since this was an implementation issue the test might be a good idea.

I'm going to run our tests, but our data repo is having some problems so it is likely the tests will fail. I'll formally review this soon, but I'm working on the testing behind the scenes so we just ask for some patience as we work on getting that fixed!

All good, updated the comment!