-
Notifications
You must be signed in to change notification settings - Fork 52
Separate ClusterlessDecodingV1 make #1467
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: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1467 +/- ##
==========================================
- Coverage 71.35% 71.13% -0.23%
==========================================
Files 114 114
Lines 13282 13308 +26
==========================================
- Hits 9477 9466 -11
- Misses 3805 3842 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
edeno
left a comment
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.
Pending conflict resolution
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.
Pull Request Overview
This PR refactors the ClusterlessDecodingV1.make method into a tripartite structure (make_fetch, make_compute, make_insert) to separate concerns and improve code organization. This pattern allows for better modularity and follows a similar approach used in other Spyglass tables like SpikeSortingRecording.
- Split the monolithic
makemethod into three focused methods for fetching data, performing computation, and inserting results - Improved parameter handling by using
.get()with defaults instead of direct dictionary unpacking - Simplified interval fetching with the
dict(nwb_dict, ...)merging pattern
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/spyglass/decoding/v1/clusterless.py | Refactored make into make_fetch, make_compute, and make_insert methods; removed unused copy import; improved parameter handling and interval fetching patterns |
| CHANGELOG.md | Added entry documenting the tripartite make refactoring for ClusterlessDecodingV1 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decoding_kwargs = decoding_kwargs or {} | ||
|
|
||
| decoding_params = model_params.get("decoding_params", dict()) | ||
| decoding_kwargs = model_params.get("decoding_kwargs", dict()) |
Copilot
AI
Nov 19, 2025
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.
Potential bug: Using .get("decoding_kwargs", dict()) will return None if the key exists in model_params but has value None, whereas the original code used decoding_kwargs or {} to convert None to an empty dict. This could cause issues if decoding_kwargs is explicitly None in the database (e.g., when checking if "is_training" not in decoding_kwargs on line 149).
To match the original behavior:
decoding_params = model_params.get("decoding_params") or {}
decoding_kwargs = model_params.get("decoding_kwargs") or {}Or alternatively:
decoding_kwargs = model_params.get("decoding_kwargs", dict()) or {}| decoding_kwargs = model_params.get("decoding_kwargs", dict()) | |
| decoding_kwargs = model_params.get("decoding_kwargs") or {} |
Checklist:
CITATION.cffaltersnippet for release notes.CHANGELOG.mdwith PR number and description.