-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add pose reader classes supporting ingestion of SLEAP tracking data #206
Conversation
One potential change to the Sleaper reader class is to remove the requirement of I will review and potentially propose a modification to the current solution. |
We discussed the possibility of the reader not needing the model path. The model path can be extracted from some meta information somewhere. |
@ttngu207 @jkbhagatio @lochhh today we were discussing file name conventions for binary SLEAP acquisition files, and we are currently leaning towards simply using the relative model path as an identifier suffix: An example file name would be: Even though it feels long, we were comparing it with other alternatives such as assigning a GUID to each trained model and it wouldn't be much different: The former is arguably much more readable and ammenable to wildcard glob search, e.g. searching for all tracking data of |
Given the above, we assume that model metadata config file lives next to the model? @glopesdev also if we go with the dataframe columns being: class | class_conf | part | part_conf | x | ywill need to slightly change this reader's |
Hi all, there should be a working version of this now, via the It should work with the example files in '/ceph/aeon/aeon/data/processed/test-enc1...' We should add a proper test for this (and other readers), and decide where it will finally live (see #226), but this provides a working solution for now. |
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.
Looks great, just have a few comments to look into before merging. I think most of them can be left for when we decide to move this reader into the core io
module, but some could be nice to do right now.
38cf625
to
8a520c1
Compare
b7a95e8
to
9975391
Compare
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.
A few more optimization and formatting questions, but otherwise looks good!
Co-authored-by: Chang Huan Lo <[email protected]> Co-authored-by: Goncalo Lopes <[email protected]>
9975391
to
44efe2d
Compare
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.
Looks good, just left some final comments for consideration and I think the refactoring of tests should be left to another PR, since they seem to be unrelated to testing this reader and possibly more related to the #228 PR.
tests/io/test_api.py
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.
Can we keep this refactoring of the tests outside of this PR? I am happy discussing and refactoring the standard imports as we discussed on #238 but would make it easier to keep track of changes if that was done in a separate PR.
@glopesdev Sounds good, read all of your most recent replies/suggestions here and agree with them all - will do them all before merging, and move the tests to #228 |
A draft of the reader for
SainsburyWellcomeCentre/aeon_experiments#241