SLEAP read user labels #79
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 98.05% 98.19% +0.14%
==========================================
Files 8 8
Lines 308 333 +25
==========================================
+ Hits 302 327 +25
Misses 6 6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Great job @lochhh!
I have two general comments on this, apart from the specifics:
- I think we need to explicitly test for the case that a .slp file contains both predicted and user-labeled instances and make sure that we load the expected things in that case (according to what you describe in the docstring of
_sleap_user_labels_to_numpy()) - The
load_poses.pyis starting to get a bit long. How about refactoring the private_functions into a separatemovement.io.utils.pyfile? If you agree with this, let's open an issue to track this (we should do it in a different PR, after this one is merged).
tests/test_unit/test_load_poses.py
Outdated
| @pytest.fixture(scope="module") | ||
| def sleap_user_labels(self): | ||
| """Return a SLEAP `Labels` object containing only | ||
| user-labeled instances. | ||
| """ | ||
| slp_file = POSE_DATA.get( | ||
| "SLEAP_three-mice_Aeon_proofread.predictions.slp" | ||
| ) | ||
| return read_labels(slp_file) | ||
|
|
||
| @pytest.fixture(scope="module") | ||
| def sleap_predicted_labels(self): | ||
| """Return a SLEAP `Labels` object containing only | ||
| predicted instances. | ||
| """ | ||
| slp_file = POSE_DATA.get("SLEAP_single-mouse_EPM.predictions.slp") | ||
| return read_labels(slp_file) |
There was a problem hiding this comment.
Currently we have two types of datasets, either all predicted or all user-labeled instances. A user may supply a .slp file with a mix of both, and we want to make sure that we handle that appropriately.
So it seems to me that we are missing a test case. Could we "mock" such a dataset by taking "SLEAP_single-mouse_EPM.predictions.slp" and converting some instances to user-labeled? I'm open to other ways of creating this test-case as well.
There was a problem hiding this comment.
I think this concerns a separate issue. For simplicity, let's consider a single-animal frame here.
Possible scenarios leading to a mix of Predicted and User-labelled Instances in a sleap.instance.LabeledFrame:
- case 1 - user "fixes" incorrectly predicted individual A:
[PredictedInstance(A)]→[PredictedInstance(A), Instance(A)] - case 2 - user "adds" missing individual A:
[]→[Instance(A)]
In either case, the current behaviour using sleap-io's Labels.numpy() is to ignore user-labelled instances:
- loading
case1.slpin movement returns[PredictedInstance(A)] - loading
case2.slpin movement returns[]
However, if the user exports case1.slp as case1.h5, and case2.slp as case2.h5, SLEAP's write_tracking_h5 prioritises the user-labelled instances, i.e., if user-labelled instance is present, select user-labelled instance, otherwise select predicted instance. Hence:
- loading
case1.h5in movement returns[Instance(A)]with NaN confidence - loading
case2.h5in movement returns[Instance(A)]with NaN confidence
I guess we should handle .slp files with mixed instance types the same way SLEAP does in write_tracking_h5, so that loading .slp and .h5 in movement return the same dataset. And perhaps, for consistency, we should leave confidence scores for user-labels-only files as NaNs, rather than setting these as 1.0...
Wrt test data, I think it's more straightforward to add a pair of .h5 and .slp files to our sample datasets.
Wdyt?
There was a problem hiding this comment.
I thought about it, and I think what you propose here makes sense and aligns better with the expectations of users. If I correct a prediction (thereby, turning it into a user-labelled instance), I expect it to be prioritised for going into the output, like write_tracking_h5 does.
Moreover, as you point out, it makes sense for the imported data to be the same, whether it came from .slp or .h5. This means that we should indeed duplicate the functionality of write_tracking_h5. Since we cannot depend on sleap, and sleap-io doesn't have it, we should write our own implementation for it.
There are two issues I foresee with this approach:
- We have to keep an eye on the
write_tracking_h5insleap. If it's behaviour changes significantly, we may need to update our own version. But I hope they won't do that, because if they do, we'll have to ship different data loaders for different versions ofsleap. But I guess this is the case already, we have to stay up-to-date with developments indeeplabcutandsleap. - As you mention, we probably have to mark the user-labelled instances with NaN confidence. This will not be a problem if we assume that user-labelling is the only process that can generate NaNs in
.slpand.h5files. If this assumption holds, we can treat NaNs coming fromsleapas having the highest confidence during preprocessing. If this assumption is not true, we need another way of keeping track of which instances are user-labelled (maybe an additional booleanDataArrayor a list of the indices of user-labelled instances stored in theDataset's metadataattrs.
In summary, I think you should go ahead with implementing this plan in this PR, and we should come to a decision on how to handle NaNs. We definitely need to keep information on which predictions we can "trust", because it will come in handy during data cleaning.
358ea10 to
c3cd58f
Compare
Co-authored-by: Niko Sirmpilatze <niko.sirbiladze@gmail.com>
2650542 to
a4d9523
Compare
| loaded. If there are multiple videos in the file, only the first one will | ||
| be used. | ||
| You can also directly load the ".slp" file. However, if the file contains | ||
| multiple videos, only the pose tracks from the first video will be loaded. |
There was a problem hiding this comment.
Should we also mention here that user-labeled instances are prioritised over the predicted ones? You have a thorough note about it in the docstring of the private function, but the users won't interface with that one, so I think it's also worth mentioning here.
There was a problem hiding this comment.
Good point! Will add that.
|
Kudos, SonarCloud Quality Gate passed!
|
Description
What is this PR
Why is this PR needed?
This PR enables
load_poses._load_from_sleap_labels_fileto load pose tracks from a SLEAP .slp file consisting of either:What does this PR do?
This PR adds:
load_poses._sleap_labels_to_numpy, a function that converts SLEAPLabelsto a NumPy array, adapted fromLabels.numpy()from thesleap_iopackage, but prioritises user-labelled instances over predicted instances when the .slp file consists of a mix of user-labelled and predicted instances. This behaviour mirrors SLEAP's approach when exporting ".h5" analysis files, so that the loading a .slp file in movement returns the same dataset as the corresponding SLEAP analysis .h5 file (i.e., exported from the same .slp file).References
This PR addresses #84
How has this PR been tested?
Tests have been written and run for the new functions added.
Does this PR require an update to the documentation?
Docstrings have been updated accordingly.
Checklist: