Skip to content

Support for using event names as IDs in PL2 files #4049

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

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

Conversation

ShijiMi-Soup
Copy link

Hi! I was having issues using the event ids of .pl2 data. The ids were all numbers and not unique (like '1', '2', ..., '1', '2', ..., which are supposed to be 'KBD01', 'KBD02', ... 'EVT01', 'EVT02'...). So I added an option to use event names as ids for .pl2 files.

Summary by copilot

This pull request introduces a new feature to allow the use of channel names as unique identifiers (use_names_as_ids) in event extractors, along with corresponding updates to ensure compatibility and proper functionality. The changes primarily affect the NeoBaseEventExtractor class and its subclasses.

Enhancements to event extraction:

  • Added use_names_as_ids parameter to NeoBaseEventExtractor: The __init__ method of NeoBaseEventExtractor now includes a use_names_as_ids parameter. When set to True, channel names are used as identifiers instead of channel IDs. An assertion ensures that channel names are unique when this option is enabled. (src/spikeinterface/extractors/neoextractors/neobaseextractor.py, src/spikeinterface/extractors/neoextractors/neobaseextractor.pyL668-R680)
  • Updated NeoEventSegment to support use_names_as_ids: The NeoEventSegment class now accepts the use_names_as_ids parameter and uses it to determine whether to index event channels by name or ID. (src/spikeinterface/extractors/neoextractors/neobaseextractor.py, src/spikeinterface/extractors/neoextractors/neobaseextractor.pyL687-R709)

Subclass-specific updates:

  • Modified Plexon2EventExtractor to default use_names_as_ids to True: The Plexon2EventExtractor subclass now defaults the use_names_as_ids parameter to True in its constructor, ensuring that channel names are used as identifiers by default for this extractor. (src/spikeinterface/extractors/neoextractors/plexon2.py, src/spikeinterface/extractors/neoextractors/plexon2.pyL126-R128)

@zm711
Copy link
Member

zm711 commented Jul 11, 2025

This seems fair to me. Our default for sorting extractors is None (but really False). So for parsimony reasons I would have the default be false and make the user purposely switch it to True.

@ShijiMi-Soup
Copy link
Author

Makes sense. I changed the default to False.

@zm711
Copy link
Member

zm711 commented Jul 11, 2025

MacOS tests keep failing, but I don't recognize this error. Let's see if @h-mayorquin recognizes it.

Plexon2RecordingTest::test_neo_annotations - TimeoutError: session server did not appear

@h-mayorquin
Copy link
Collaborator

No idea why is this. I think we should skip this test on mac as this is a feature that is useful for the user and wine/plexon has been a pain for a while. We don't really have resources to dig deeper into this as far as I am aware.

As for the default, let's discuss this on the meeting I vaguely remember that Sam prefers to keep None for provenance but I might be misremembering. That is, False or True the user chosen it but None means the user did not choose if that makes sense.

@zm711
Copy link
Member

zm711 commented Jul 18, 2025

That is, False or True the user chosen it but None means the user did not choose if that makes sense.

Yeah I think you're right on that. None defaults to False behavior, but for provenance keeping None is fine since the other extractors do it.

I think people would really only trust you to muck around with the testing for Mac at that level. so happy to figure out how to fix this at a maintenance meeting.

@zm711
Copy link
Member

zm711 commented Jul 18, 2025

@ShijiMi-Soup we will ping you once we have testing fixed for any final patches to the PR :)

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.

3 participants