Add ability to set SortGroups based on electrode table column #1438
Conversation
| sg_key = dict( | ||
| nwb_file_name=nwb_file_name, | ||
| sort_group_id=group_id, | ||
| sort_reference_electrode_id=-1, # TODO make this general? Berke Lab is always -1 for reference electrode |
There was a problem hiding this comment.
Note I don't currently do anything with sort_reference_electrode_id besides set it to -1 because for Berke Lab it is always -1. But I can change this
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1438 +/- ##
==========================================
- Coverage 70.50% 70.25% -0.26%
==========================================
Files 104 104
Lines 12657 12710 +53
==========================================
+ Hits 8924 8929 +5
- Misses 3733 3781 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to create SortGroups based on arbitrary electrode table columns, particularly useful for the Berke Lab workflow. The implementation allows users to group electrodes by any column in the NWB file's electrode table (e.g., "intan_channel_number") rather than being limited to grouping by shank.
Key changes:
- Added
get_group_by_electrode_table_column()utility function for flexible electrode grouping - Added
set_group_by_electrode_table_column()class method to SortGroup table - Implemented validation for existing entries and custom sort group IDs
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/spyglass/spikesorting/v1/recording.py | Adds new class method set_group_by_electrode_table_column() to SortGroup with entry conflict handling |
| src/spyglass/spikesorting/utils.py | Implements core logic for grouping electrodes by arbitrary table columns with bad channel filtering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| sg_key = dict( | ||
| nwb_file_name=nwb_file_name, | ||
| sort_group_id=group_id, | ||
| sort_reference_electrode_id=-1, # TODO make this general? Berke Lab is always -1 for reference electrode |
There was a problem hiding this comment.
The hardcoded reference electrode ID of -1 should be exposed as a parameter with a default value rather than being hardcoded. Consider adding a sort_reference_electrode_id parameter to both get_group_by_electrode_table_column() and set_group_by_electrode_table_column() with a default of -1.
|
|
||
| # Optionally remove bad channels | ||
| if remove_bad_channels: | ||
| bad_subset = subset[subset["bad_channel"] == 1] |
There was a problem hiding this comment.
The bad channel check uses == 1 but the filtering later uses == 0. This assumes bad_channel is always 0 or 1, but the comparison with != 0 in the docstring (line 123, 168) suggests it could have other values. Consider using != 0 for consistency with the documented behavior.
| bad_subset = subset[subset["bad_channel"] == 1] | |
| bad_subset = subset[subset["bad_channel"] != 0] |
| f"Removing bad channels from group {group_id}: " | ||
| f"{bad_subset.index.tolist() if use_index else bad_subset[column].tolist()}" | ||
| ) | ||
| subset = subset[subset["bad_channel"] == 0] |
There was a problem hiding this comment.
This filtering assumes bad_channel is always 0 or 1, but the docstrings indicate 'bad_channel != 0' should be removed. Use subset = subset[subset['bad_channel'] == 0] should be subset = subset[subset['bad_channel'] != 0] for the bad channels identification, or change the good channels filter to check for explicit 0 values consistently with documentation.
| nwb_file_name: str, | ||
| column: str, | ||
| groups: list[list], | ||
| sort_group_ids: list[int] = None, |
There was a problem hiding this comment.
Using mutable default argument None is correct, but the type hint should be Optional[list[int]] or list[int] | None for Python 3.10+ to properly indicate the parameter is optional.
| sort_group_ids: list[int] = None, | |
| sort_group_ids: Optional[list[int]] = None, |
| nwb_file_name: str, | ||
| column: str, | ||
| groups: list[list], | ||
| sort_group_ids: list[int] = None, |
There was a problem hiding this comment.
The type hint should be Optional[list[int]] or list[int] | None for Python 3.10+ to properly indicate the parameter accepts None.
|
@calderast Is this still a draft or did you want us to review? |
2 participants
Description
Add ability to set SortGroups based on electrode table column. Helpful for Berke Lab users, could be helpful to others also
Checklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.