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

Fix-ate plexon signal streams #1524

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

h-mayorquin
Copy link
Contributor

@h-mayorquin h-mayorquin commented Aug 15, 2024

This PR solves two things:

  1. Should solve Plexon signal stream_ids are not cosistent across reads #1523 by using consistent stream_ids defined by plexon channel prefixes that are stable (see here
  2. It adds human readable and meaningful stream names. Right now they are called "Signal + {stream_id}" and after this PR, they will be:
            channel_prefix_to_stream_name = {
                "WB": "WB-Wideband",
                "FP": "FPl-Low Pass Filtered ",
                "SP": "SPKC-High Pass Filtered",
                "AI": "AI-Auxiliary Input",
            }

Not that this moves forward the poject in SpikeInterface/spikeinterface#3197 that aims to have "logical streams" and relegate "buffer streams" to an internal implementation detail of the raw io API.

zm711
zm711 previously approved these changes Aug 15, 2024
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me. I'll leave this open until next week to see if @alejoe91 or @samuelgarcia want to take a look before merging. :)

neo/rawio/plexonrawio.py Outdated Show resolved Hide resolved
@zm711 zm711 added this to the 0.14.0 milestone Aug 16, 2024
@h-mayorquin
Copy link
Contributor Author

OK, the test file that we have is quite limited. I would need this:
https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/137

@zm711
Copy link
Contributor

zm711 commented Aug 21, 2024

OK, the test file that we have is quite limited. I would need this:
https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/pulls/137

Merged @h-mayorquin

I'll re-run tests so you can confirm your changes work with the new demo file.

@zm711
Copy link
Contributor

zm711 commented Aug 21, 2024

The rtd failure should also be looked at. (Since plexon is used for the doc build!)

@zm711 zm711 dismissed their stale review August 22, 2024 14:23

tests now failing.

@h-mayorquin
Copy link
Contributor Author

Fixed the test and added a new one.

neo/rawio/plexonrawio.py Outdated Show resolved Hide resolved
Comment on lines 301 to 302
# The users of plexon can modify the channel names, is not common but in that case
# We assign the channel_prefix both as stream_name and stream_id
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean?

So they can re-name a channel to not be WB, FB, SP, AI etc... What part can they actually rename? Do we need to protect against that better or could we make this comment clearer to what the renaming actually entails?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The can modify the prefix. For example, in the test file that we had before I added the new one, the channel is named V1.

In that case I am adding V as both the stream name and id. This can't be generalized but the user changed the prefix for some reason so they should know what it means in neo when they see the header and in spikeinterface when they get the assertion that the stream was not found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I think I see what you mean! Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Let me know if there is an improvement to the comment that I could make.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would make a little more sense in the comment to clarify channel name vs channel prefix. In the first part you say channel names, then you say we take the channel prefix. So making it clear: can the user modify the prefix, all of the name, the numbering of the channel.

Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still good by me. @alejoe91 or @samuelgarcia want to take a quick look as well?

neo/rawio/plexonrawio.py Outdated Show resolved Hide resolved
@zm711 zm711 merged commit 9afcc15 into NeuralEnsemble:master Aug 28, 2024
3 checks passed
@zm711 zm711 modified the milestones: 0.14.0, 0.13.3 Aug 28, 2024
@h-mayorquin h-mayorquin deleted the fix_plexon_streams branch August 28, 2024 13:27
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.

2 participants