Skip to content

Conversation

CeliaLrt
Copy link

@CeliaLrt CeliaLrt commented Aug 15, 2025

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

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:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copy link

@sfmig sfmig requested a review from niksirbi August 20, 2025 09:42
Copy link

codecov bot commented Aug 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (3edbf15) to head (2a5a356).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #658   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         1981      1981           
=========================================
  Hits          1981      1981           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfmig sfmig self-requested a review August 20, 2025 15:29
Copy link
Member

@niksirbi niksirbi left a 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).

@sfmig sfmig removed their request for review August 27, 2025 14:16
@niksirbi
Copy link
Member

Hey @CeliaLrt, just checking in to see when you might have a chance to continue with this PR.
No rush at all — I’m mainly trying to keep track of open PRs and plan my review time accordingly.
If for any reason you can't find the time, let me know and I can take care of finalising this PR.

@CeliaLrt
Copy link
Author

Hi @niksirbi, sorry for the delay! I'll continue working on it this week

@CeliaLrt CeliaLrt changed the title Add documentation to specify that skeleton must be identical for all … Add documentation to specify Movement require identical keypoints (#150) Oct 2, 2025
@CeliaLrt CeliaLrt changed the title Add documentation to specify Movement require identical keypoints (#150) Add documentation to specify Movement require identical keypoints ([#150](https://github.com/neuroinformatics-unit/movement/issues/150)) Oct 2, 2025
@CeliaLrt CeliaLrt changed the title Add documentation to specify Movement require identical keypoints ([#150](https://github.com/neuroinformatics-unit/movement/issues/150)) Add documentation to specify Movement require identical keypoints Add pose loading check (#150) Oct 2, 2025
@CeliaLrt CeliaLrt changed the title Add documentation to specify Movement require identical keypoints Add pose loading check (#150) Add documentation to specify Movement require identical keypoints (#150) Oct 2, 2025
Copy link

@CeliaLrt CeliaLrt requested a review from niksirbi October 20, 2025 11:21
Copy link
Member

@niksirbi niksirbi left a 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!

@CeliaLrt
Copy link
Author

Hi @niksirbi, ok great! Thank you for the guidance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle the case where not all individuals have the same keypoints.

2 participants