-
Notifications
You must be signed in to change notification settings - Fork 216
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
base: main
Are you sure you want to change the base?
Conversation
- fetches the xml with channel groups - automatically adds it as a group in the recording - parses the real colors and positions (offsets) directly from Neuroscope and adds them as properties of the object (handy for ephyviewer) Will allow for parallel spike sorting, processing... as if it were separate recordings/structures (for now it's only anatomy-based, but we could do the same with the "spike groups" identified after spike sorting by Neuroscope).
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Thanks for the contribution @theodchrn. Super cool. I think @h-mayorquin recently did something similar for our Intan reading capabilities. Maybe he would be good to give this a glance over? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
I am unfamiliar with Neuroscope so I just made some general comments.
Questions:
what do the channel groups mean in this format? what do they stand for? Same for discarded channel.
For the maintainers.
Maybe both here and in Intan we should have a light version and only load the groups -and everything else- when all_annotations
is true. This last suggestion should not stop your PR and we can do this after, this message is intended for other maintainers.
|
||
return channel_groups, kept_channels, discarded_channels, anatomycolors | ||
|
||
def split_recording_by_channel_groups(self): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
group_ids[numbers] = group_id # Assign group_id to the positions in `numbers` | ||
self.set_property("group", group_ids) | ||
discarded_ppty = np.full(n, False, dtype=bool) | ||
discarded_ppty[discarded_channels] = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does discarded channel mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it just means it wont be shown in the final viewer and was probably considered as noise/unplugged channel on the eib
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())) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Hey @h-mayorquin ! Thanks for the review :) I'll try my best to answer those.
|
Thanks for your answers.
Can they be anything else than brain areas? if not, then the property should be named "brain_area". "groups" in SpikeInterface are usually vaguely things that you would sort separately (e.g. a shank in a multi shank neuropixel probe).
Mmm I am unsure about this. Is this general neuroscope or specific to your lab workflow? |
Yes it could be something else than brain area - for example in my lab we have EKG and EMG recordings as well, you could probably add breathing data... I called them groups because (at least in my use case) i run the sorting in SpikeInterface based on those groups (they could represent a shank!) - I just skip the groups that are not brain areas.
Tbh i dont know. I assumed it was general neuroscope, but maybe it's only us! In my opinion it's also one of the main advantages of Neuroscope, as you directly visualize relevant data, already sorted for the different brain/body areas you're recording. Let me know if this is not clear enough! English is not my 1st language and I am not very familiar with neuroscope usage outside of of my lab scope. |
Then, maybe we should call them It would be nice if we could have more meaningful names but it seems that those properties mean different things for different users. I need to think more about this but i would like someone who works with visualization or knows neuroscope to review this as I think we need more knowledge for this system so we don't overfit to a visualization workflow or add things that are not meaningful in other contexts. Maybe a compromise so the user can have facilities but at the same time we don't make strong commitments is to add a new method "prepare_neuroscope_for_ephyviewer" which loads these properties but the user needs to call explicitly (I am not sure this work with ephyviewer). I am slightly against adding yet another keyword because it will crash with all_annotations |
Hi!
This is the first time i'm doing a pull request on an "official" project, and don't really know how to do it. If you have any remark/advice on how to do it properly i'd be glad to have them!
Basically i would just like to share my small improvements on reading neuroscope-type data (amplifier.dat coupled with amplifier.xml). I don't think this format is widely used to date, but for the few of us that still do I think this might be a step forward to using SpikeInterface more easily.
What I've added for my personal use and think might benefit everyone:
Will allow for parallel spike sorting, processing... as if it were separate recordings/structures (for now it's only anatomy-based, but we could do the same with the "spike groups" identified after spike sorting by Neuroscope).