Skip to content

Better Neuroscope support. #3862

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

Merged
merged 15 commits into from
Jul 3, 2025
Merged
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions src/spikeinterface/extractors/neoextractors/neuroscope.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import warnings
from pathlib import Path
from typing import Union, Optional
from xml.etree import ElementTree as Etree

import numpy as np

Expand Down Expand Up @@ -64,6 +65,8 @@ def __init__(
if xml_file_path is not None:
xml_file_path = str(Path(xml_file_path).absolute())
self._kwargs.update(dict(file_path=str(Path(file_path).absolute()), xml_file_path=xml_file_path))
self.xml_file_path = xml_file_path if xml_file_path is not None else Path(file_path).with_suffix(".xml")
self.split_recording_by_channel_groups()

@classmethod
def map_to_neo_kwargs(cls, file_path, xml_file_path=None):
Expand All @@ -78,6 +81,59 @@ def map_to_neo_kwargs(cls, file_path, xml_file_path=None):

return neo_kwargs

def _parse_xml_file(self, xml_file_path):
"""
Comes from NeuroPhy package by Diba Lab
"""
tree = Etree.parse(xml_file_path)
myroot = tree.getroot()

for sf in myroot.findall("acquisitionSystem"):
n_channels = int(sf.find("nChannels").text)

channel_groups, skipped_channels, anatomycolors = [], [], {}
for x in myroot.findall("anatomicalDescription"):
for y in x.findall("channelGroups"):
for z in y.findall("group"):
chan_group = []
for chan in z.findall("channel"):
if int(chan.attrib["skip"]) == 1:
skipped_channels.append(int(chan.text))

chan_group.append(int(chan.text))
if chan_group:
channel_groups.append(np.array(chan_group))

for x in myroot.findall("neuroscope"):
for y in x.findall("channels"):
for i, z in enumerate(y.findall("channelColors")):
try:
channel_id = str(z.find("channel").text)
color = z.find("color").text

except AttributeError:
channel_id = i
color = "#0080ff"
anatomycolors[channel_id] = color

discarded_channels = np.setdiff1d(np.arange(n_channels), np.concatenate(channel_groups))
kept_channels = np.setdiff1d(np.arange(n_channels), np.concatenate([skipped_channels, discarded_channels]))

return channel_groups, kept_channels, discarded_channels, anatomycolors

def split_recording_by_channel_groups(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be private and have a name other than split which has a different meaning on SI. Maybe set_groups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by private ? i'm not super familiar with advanced objects. Is it the __split_recording caracter ?

Copy link
Member

Choose a reason for hiding this comment

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

private in general would be
_set_groups so one underscore. The __ leads to name mangling, which I personally hate. But _ underscore is a flag to the user that this function is for internal use and that the implementation and arguments could change at any time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sounds fair! i'll push a new commit for that

n = self.get_num_channels()
group_ids = np.full(n, -1, dtype=int) # Initialize all positions to -1

channel_groups, kept_channels, discarded_channels, colors = self._parse_xml_file(self.xml_file_path)
for group_id, numbers in enumerate(channel_groups):
group_ids[numbers] = group_id # Assign group_id to the positions in `numbers`
self.set_property("group", group_ids)
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @h-mayorquin for neuroscope_group

discarded_ppty = np.full(n, False, dtype=bool)
discarded_ppty[discarded_channels] = True
self.set_property("discarded_channels", discarded_ppty)
self.set_property("colors", values=list(colors.values()), ids=list(colors.keys()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unsure about adding colors here but I think that @samuelgarcia and @alejoe91 are more familiar with viewers so I defer to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's mostly a gimmick for reproducibility of data visualisation with my lab ahah. I use it later in a custom ephyviewer script (not shown here, but maybe i should share it as well ?)

Copy link
Member

Choose a reason for hiding this comment

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

I think we just need to understand is this something all users should think about or is this super specific. If colors are always generated in a consistent (at least to the user) way then it might be worth adding (again with Heberto's final suggestion to fit this in with all_annotations for a future PR).

Basically we want properties set here to be truly meaningful to all users. Other properties can be set on a per-user basis with our property setting machinery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! In neuroscope it's always generated in a consistent manner, hardcoded in the paired xml file.

Copy link
Member

Choose a reason for hiding this comment

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

OK for color but maybe we could rename this to neuroscope_color no ?



class NeuroScopeSortingExtractor(BaseSorting):
"""
Expand Down