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

[Discussion] Consistency of stream_index in neo.rawio #1604

Open
h-mayorquin opened this issue Dec 5, 2024 · 3 comments
Open

[Discussion] Consistency of stream_index in neo.rawio #1604

h-mayorquin opened this issue Dec 5, 2024 · 3 comments
Milestone

Comments

@h-mayorquin
Copy link
Contributor

Context

In PR #1601, @zm711 raised concerns about the usage and stability of stream_index as a public API in neo.rawio. The primary concern is whether changes to the ordering of streams could introduce breaking changes for existing users relying on stream_index. Specifically:

A user switching to a newer version of the library may find their code broken if the stream_index mapping changes (e.g., stream_index=15 no longer refers to the same stream).

The core of the problem is that while stream_index is exposed as a primary access method in rawio, it appears to lack a formal guarantee or contract that it remains consistent across versions.

Discussion

  • In practice, the stream_index is derived from the signal streams header and that does not have a fixed meaning across different instances of the same format. For example, in Plexon, some streams may not be present, altering the indices dynamically. This is in contrast to id and name, which are unique and stable identifiers for streams. My question here is, if the indexes were meant to be consistent, why would we need stream ids?

  • Exposing stream_index at the API level seems to have been a design choice based on the natural order semantics of segment index and block index. However, this has led to unintended reliance on stream_index which has mentioned it is not stable.

  • Most code that I have found handles the dynamic nature of the stream_indexes just fine (I gave examples here). Spikeinterface for example, does not assume that stream_index is constant and exposes the stream_id and stream_name but if there are direct users of the raw.io API it will make it unstable for them.

Thoughts?

@zm711
Copy link
Contributor

zm711 commented Dec 13, 2024

We started the discussion today but plan to have a larger discussion at our next meeting. The plan at that meeting will be to discuss what parts of the io and rawio should actually be public and what should be private. And then decide what contract we wish for the IO and RawIO layers moving forward.

Today we just decided that we wanted to carefully go over things and keep these types of issues in mind as we finalize. We also discussed potentially making a series of "breaking" changes at the RawIO level to make functions that should have been private, private so that moving forward we make sure users know what shouldn't change and what is free to change.

@zm711 zm711 modified the milestones: 0.14.0, 1.0.0 Dec 13, 2024
@samuelgarcia
Copy link
Contributor

Hi.
Very good point.

I do not remember why I choose stream_index instead of stream_id for accessing data.

Note that stream_id can also be altered along the way by hanges in neo itself or changes in the format itself (some of then change often).

Note that it would be very easy to add get_analogsignal_chunk(..., stream_id=...) because all implementation are private _get_analogsignal_chunk()

@h-mayorquin
Copy link
Contributor Author

Note that it would be very easy to add get_analogsignal_chunk(..., stream_id=...) because all implementation are private _get_analogsignal_chunk()

yeah, seems like the way to go.

Let me know if @zm711, @apdavison and @alejoe91 agree with this and I can help with this if you want.

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

No branches or pull requests

3 participants