Skip to content

Conversation

@lochhh
Copy link
Member

@lochhh lochhh commented Nov 17, 2025

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Closes #241 #210

What does this PR do?

  • Reorganises the datasets validators (ValidPosesInputs, ValidBboxesInputs) to subclass from _BaseDatasetInputs
  • Rewrites the relevant tests to reduce duplication - what have been tested in the parent class are not repeated in the subclasses
  • Enforces uniqueness in keypoint_names, individual_names supplied (in addition to length check)
  • Replaces hardcoded index numbers (e.g. fragile assumption that -1 refers to the individuals dim) by inferring based on DIM_NAMES
  • Renames dataset validator classes to better reflect what they actually are (input validators, but not datasets)
  • Moves _ds_from_valid_data into the input validator classes as to_dataset for converting validated inputs into xr.Datasets

References

#241, #210 part of #667

How has this PR been tested?

Unit tests have been rewritten

Is this a breaking change?

Yes, the Validators have been renamed to ValidPosesInputs and ValidBboxesInputs

Does this PR require an update to the documentation?

Not until #722 is finalised

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

@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (96c28a7) to head (50af275).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #706   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           34        34           
  Lines         2104      2110    +6     
=========================================
+ Hits          2104      2110    +6     

☔ 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.

@lochhh lochhh force-pushed the unify-pose-bbox branch 2 times, most recently from 0a33e5c to 3233aef Compare December 1, 2025 15:17
@lochhh lochhh marked this pull request as ready for review December 1, 2025 15:26
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 1, 2025

@lochhh lochhh requested a review from a team December 1, 2025 17:02
@sfmig sfmig self-requested a review December 8, 2025 11:19
@niksirbi niksirbi removed the request for review from a team December 8, 2025 11:23
Copy link
Member

@sfmig sfmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing job, thanks @lochhh! 🚀

I like the modularity in both the validators and their tests, it makes everything so much more readable.

My comments are mostly about variable names, I'm curious about your opinion and don't feel too strongly about them either. Let me know thoughts.

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.

I've also had a look at this, out of interest, and had very little to add (see inline comments). Adding my approval to the list.

@lochhh lochhh force-pushed the unify-pose-bbox branch 2 times, most recently from d6625ba to 7870022 Compare January 6, 2026 19:38
@lochhh
Copy link
Member Author

lochhh commented Jan 6, 2026

Thanks @sfmig and @niksirbi for your input!
The main changes are

  • renaming validators to _BaseDatasetInputs, ValidPosesInputs, ValidBboxesInputs
  • adding the abstract method to_dataset() to_BaseDatasetInputs
  • moving _ds_from_valid_data in load_poses, load_bboxes into the respective ValidInputs.to_dataset implementations and adding the unit tests
  • rephrasing error messages, docstrings

@lochhh lochhh requested a review from sfmig January 6, 2026 19:40
@lochhh lochhh linked an issue Jan 7, 2026 that may be closed by this pull request
@lochhh lochhh force-pushed the unify-pose-bbox branch 3 times, most recently from 5b42f67 to c8e4019 Compare January 8, 2026 16:22
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 8, 2026

@niksirbi niksirbi added this pull request to the merge queue Jan 9, 2026
Merged via the queue into main with commit 887bd02 Jan 9, 2026
25 checks passed
@niksirbi niksirbi deleted the unify-pose-bbox branch January 9, 2026 11:36
@niksirbi niksirbi mentioned this pull request Jan 9, 2026
4 tasks
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.

Refactor validators to reduce code duplication between bboxes and poses Need to re-design dataset validators?

4 participants