- 
                Notifications
    You must be signed in to change notification settings 
- Fork 72
Add documentation to specify Movement require identical keypoints (#150) #658
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
Conversation
| 
 | 
| Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff            @@
##              main      #658   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         1997      1997           
=========================================
  Hits          1997      1997           ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
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.
Thanks for the contribution @CeliaLrt!
I like that you also link to the relevant place in the DeepLabCut docs, that's good practice!
I've added some small suggestions for improvement.
Other than that, this PR is now out-of-sync with the main branch, because @sfmig made some changes to the Input/Output guide after you branched off main for this PR. This conflict can be dealt with by merging the latest main branch into yours, but this can be a tricky operation if you are new to git. Let me know if you need help with that, I can also do the merging for you after you address my inline comments.
Moreover, please update the description of this PR with a summary of the changes being made and a link to the relevant issue #150. This is good for posterity's sake (when our future selves will try to remember what this PR was doing).
| Hey @CeliaLrt, just checking in to see when you might have a chance to continue with this PR. | 
| Hi @niksirbi, sorry for the delay! I'll continue working on it this week | 
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.
This looks good now @CeliaLrt, thanks for implementing my suggestions!
Note that some of the CI checks are failing for reasons unrelated to the changes you've made (The internet is having some rough time right now because of issues with AWS). I will merge your PR as soon as those internet issues get resolved.
Congrats on your first movement contribution!
| Hi @niksirbi, ok great! Thank you for the guidance | 
Co-authored-by: Niko Sirmpilatze <[email protected]>
Co-authored-by: Niko Sirmpilatze <[email protected]>
765b5c7    to
    f436d2c      
    Compare
  
    | 
 | 
560c325
    


What is this PR
Why is this PR needed?
This PR addresses issue #150, to specify to users that they can only load pose data where all individuals have the same set of keypoints (i.e. same skeleton).
What does this PR do?
Specify movement require identical keypoint for all objects in the user guide documentation (input/output section) and in API reference documentation (from_dlc_file() function in movement.io.load_poses section).
References
• Fixes: #150
Is this a breaking change?
No
Checklist: