-
Notifications
You must be signed in to change notification settings - Fork 11
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
Adding support for more .nwb training data to SLEAP #101
Adding support for more .nwb training data to SLEAP #101
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
- Coverage 96.04% 95.42% -0.62%
==========================================
Files 17 17
Lines 2022 2034 +12
==========================================
- Hits 1942 1941 -1
- Misses 80 93 +13 ☔ View full report in Codecov by Sentry. |
…res' into kloding/add_nwb_training_structures
sleap_io/io/main.py
Outdated
@@ -47,7 +47,7 @@ def save_slp( | |||
return slp.write_labels(filename, labels, embed=embed) | |||
|
|||
|
|||
def load_nwb(filename: str) -> Labels: | |||
def load_nwb(filename: str, format: str) -> Labels: |
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.
Suggestion: since we only have two "formats", it might be easier to just make this a boolean. For example:
def load_nwb(filename: str, as_training: bool | None = None):
if as_training is None:
as_training = ... # try to auto-detect based on whether there are tracks or user-labeled instances
if as_training:
return nwb.read_nwb_training(filename)
else:
return nwb.read_nwb(filename)
And maybe we want to rename nwb.read_nwb
to nwb.read_nwb_predictions
for clarity?
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.
PS: We probably want to keep nwb.read_nwb
and anything else that existed before as an alias for backwards compatibility.
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.
Definitely cannot commit a 16 MB test file!! Shoot for 1-2 MB max by reducing the contents of the file to the minimal subset that still allows for testing the functionality. If not possible, then maybe will need to generate the testing artifacts dynamically (e.g., converting from the base SLP file and saving to a temp dir).
tests/data/slp/minimal_instance.slp
Outdated
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.
Why is this getting changed? It's not a part of this PR's scope but is used in a lot of the rest of the package.
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.
Is this new? Why can't we use one of the other SLP files in https://github.com/talmolab/sleap-io/tree/main/tests/data/slp?
Description
Right now, data from NWB (Neurodata without Borders) is partially supported in
sleap-io
. ThePoseEstimation
andPoseEstimationSeries
data structures are supported, but theTrainingFrame
,TrainingFrames
,PoseTraining
, andSourceVideos
structures are not. These data structures correspond to data structures in SLEAP as shown in rly/ndx-pose#24. I will add support for these data types insleap-io
by allowing the user to save and load them.My plan is to update the
save_nwb
andload_nwb
functions inio/main.py
to allow support for classes besidesLabels
. I will also update theread_nwb
function inio/nwb.py
so that it can read NWB data structures and return the corresponding SLEAP structures. So far, I have written a functionconvert_nwb
that uses NWB objects to initialize corresponding SLEAP objects with the same attributes. https://github.com/keyaloding/io_fork/blob/70a34ede294d3c783d1758c55c1dd9752e29f1e7/sleap_io/io/nwb.py#L37-L72. I will write new tests that confirm that the attributes of the SLEAP data structures that are initialized have the same attributes as the NWB data structures used to create them.I will also update the documentation to include information about how to use NWB data in SLEAP.
Types of changes
Does this address any currently open issues?
#100, #86, rly/ndx-pose#29
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️