Skip to content
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.

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

Correctly load marker streams from .xdf files #464

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

bkloeckl
Copy link
Contributor

Fixes #451 in order to load XDF containing multi-channel marker stream. Also addresses some minor issues to recognize different names of marker types and auto-convert stream dtype to supported types if possible.

@cbrnr
Copy link
Owner

cbrnr commented Jan 14, 2025

Unfortunately, this PR breaks some of the XDF test files available here: https://osf.io/uc7wn/ (e.g., xdf_003.xdf). I think it would be important to avoid any regressions.

I think we should discuss what should be interpreted as a marker stream. In main, only streams with nominal_srate == 0 and type == "Markers" are recognized as markers (independently of the channel_format field, which contains the dtype of the stream).

The most common marker streams have nominal_srate == 0, type == "Markers", channel_count == 1, and channel_format == "string". These work fine. We might even consider removing the type == "Markers" requirement if both nominal_srate == 0 and channel_format == "string".

In this PR, you are adding more than one change to the existing logic. The linked test file contains a stream with

  • type == "Marker" (no "s" at the end!)
  • channel_format == "string"
  • nominal_srate == 1000
  • channel_count == 2

Apart from the type (which is probably not important anyway), you are trying to add support for marker streams with nominal_srate != 0 and a channel_count != 1 at the same time. I think we should probably go step by step here to make sure that nothing breaks.

I think adding support for channel_count > 1 should be relatively straightforward. Adding support for nominal_srate > 0 might be the next step, taking care to discard empty strings. Finally, there could also be marker streams with channelformat != "string", i.e. int{8,16,32,64}, float32, and double64 (see https://github.com/sccn/xdf/wiki/Specifications#streamheader-chunk), but nominal_srate == 0. The example xdf_003.xdf contains such a stream (channel_format == "double64"). This is quite common, but a bit tricky, because if we convert such a stream to annotations, we're losing the numerical values (as they are converted to strings, since annotations only have string descriptions in addition to onset and duration).

So in summary, I think there is still a lot to discuss before implementing changes. I'd also be very happy for any input to my thoughts on how to best handle marker streams. I'm tagging @chkothe, @sappelhoff, @tstenner, and @cboulay here – what are your thoughts on this issue?

@sappelhoff
Copy link
Contributor

sappelhoff commented Jan 14, 2025

I think we should probably go step by step here to make sure that nothing breaks.

agreed

However I think one of the biggest problems is this: https://github.com/sccn/xdf/wiki/Markers-Meta-Data

image

So anything we do is just based on "our whim". Basically, it's the big XDF tools which get used by most people that sort of dictate what is going to be recorded/processed, and how (For example LabRecorder or pyxdf, etc.).

I think it's fine to leave a "standard" or a "format" relatively open and see how users' needs fill the landscape bit by bit, but at some point it would also be important to go back to the specification and set some rules that have emerged and that are now battle tested into stone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot load XDF containing multi-channel marker stream
3 participants