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

Propagate unit_electrode_indices to SortingInterface #1124

Merged
merged 14 commits into from
Nov 15, 2024

Conversation

h-mayorquin
Copy link
Collaborator

This will allow the user to pass a mapping between units and the electrodes on the electrode table. Eventually I will use this to add a more robust solution like here:

#1112 (comment)

@h-mayorquin h-mayorquin self-assigned this Nov 5, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review November 5, 2024 22:20
Copy link
Member

@pauladkisson pauladkisson 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.

What do you think about adding some kind of how-to explaining what to do if you have 2 probes and run kilosort like in #1112?

I think it could be an effective way to communicate to users about this new feature.

@@ -312,9 +313,15 @@ def add_to_nwbfile(
units_name : str, default: 'units'
The name of the units table. If write_as=='units', then units_name must also be 'units'.
units_description : str, default: 'Autogenerated by neuroconv.'
unit_electrode_indices : list of lists of int, optional
A list of lists of integers indicating the indices of the electrodes that each unit is associated with.
Copy link
Member

Choose a reason for hiding this comment

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

This corresponds to the sparsity in spikeinterface right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, in practice they are. Although, conceptually sparsity is a constrained channel representation (so an electronic thing) whereas this is meant to map to the representation of physical electrodes.

@pauladkisson
Copy link
Member

What do you think about adding some kind of how-to explaining what to do if you have 2 probes and run kilosort like in #1112?

Actually, if this is supposed to be used in SortedRecordingConverter specifically, maybe just a conversion gallery entry for that one would be sufficient...

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Nov 15, 2024

What do you think about adding some kind of how-to explaining what to do if you have 2 probes and run kilosort like in #1112?

Actually, if this is supposed to be used in SortedRecordingConverter specifically, maybe just a conversion gallery entry for that one would be sufficient...

Yeah, I will* be adding a tutorial there.

@h-mayorquin h-mayorquin merged commit a608e90 into main Nov 15, 2024
40 checks passed
@h-mayorquin h-mayorquin deleted the add_electrode_disambiguation_in_sorting_interfaces branch November 15, 2024 18:24
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.67%. Comparing base (64fb9e0) to head (ef50020).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1124      +/-   ##
==========================================
+ Coverage   90.58%   90.67%   +0.08%     
==========================================
  Files         128      129       +1     
  Lines        8011     8182     +171     
==========================================
+ Hits         7257     7419     +162     
- Misses        754      763       +9     
Flag Coverage Δ
unittests 90.67% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...nterfaces/ecephys/basesortingextractorinterface.py 80.19% <100.00%> (+0.40%) ⬆️
...c/neuroconv/tools/spikeinterface/spikeinterface.py 91.11% <100.00%> (+0.06%) ⬆️

... and 1 file with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants