Skip to content

Bvh loader#929

Open
Siliconedioxyde wants to merge 6 commits intoneuroinformatics-unit:mainfrom
Siliconedioxyde:bvh-loader
Open

Bvh loader#929
Siliconedioxyde wants to merge 6 commits intoneuroinformatics-unit:mainfrom
Siliconedioxyde:bvh-loader

Conversation

@Siliconedioxyde
Copy link
Copy Markdown

@Siliconedioxyde Siliconedioxyde commented Mar 26, 2026

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

This PR adds initial I/O functionality for BVH formats and establishes a foundation of reliable test coverage. It ensures the loader works correctly with both simple and relatively more complex BVH fixtures generated locally.

What does this PR do?

  1. Implements a from_bvh_file function that loads BVH files using bvhio into a movement dataset, along with a basic validator.
  2. Adds minimal BVH test files and comprehensive test units.

References

closes #299

How has this PR been tested?

This PR has been tested with a dedicated set of unit tests covering BVH loader functionality:

-Dataset validation:
test_load_from_bvh_file checks that loading both simple and complex BVH fixtures produces a valid xarray.Dataset
with correct dimensions, coordinates, and metadata (fps, file_path, source_software, time_unit).

-Joint hierarchy:
test_bvh_joint_names verifies that the joint names parsed from the BVH hierarchy match the expected structure for both
fixtures.

-FPS handling:
test_bvh_fps_from_frame_time ensures fps is correctly computed from the Frame Time field.
test_bvh_fps_none confirms that when fps=None is passed, the loader still computes fps from the file.

-Validation checks:
test_valid_bvh_file confirms that a well‑formed BVH passes validation.
test_invalid_bvh_no_hierarchy and test_invalid_bvh_no_motion assert that missing required sections raise ValueError.
test_invalid_bvh_wrong_extension ensures files with incorrect extensions fail validation.

All BVH loader tests pass. The next aim would be to add integration tests after the addition perhaps of some sample BVH data on the GIN repo.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Yes, the documentation should be updated to include the new function that allows the user to load BVH files. I will do that after finishing.

Next steps

  • Add integration tests
  • Update documentation to include BVH loader usage

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

@sonarqubecloud
Copy link
Copy Markdown

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.

Add support for optical marker-based motion capture data

1 participant