Simplify and expand kinematic tests for bboxes

sfmig commented 1 month ago

Description

What is this PR

[ ] Bug fix
[x] Addition of a new feature
[ ] Other

Why is this PR needed? To ensure our existing kinematics methods play nicely with bboxes datasets.

What does this PR do?

Simplifies kinematics tests
- Refactored tests for displacement, velocity and acceleration into a single parametrised test for computing kinematic variables
- Separated tests for valid and invalid datasets
- Added a test for a simple motion for which kinematics can be easily derived.
- Removed some hardcoded bits from the fixtures
Expands kinematics tests to valid and invalid bboxes datasets
Renames internal method for explicitness _compute_approximate_derivative ---> _compute_approximate_time_derivative
- the reasoning being this makes it a bit more clear to developers and contributors how our wrapper to compute the approximate derivate and xarray's method differentiate are distinct. With differentiate you can compute the derivative along any single-coordinate dimension, not necessarily time.

Question

Should we use xarray's differentiate in our kinematics wrappers?

I am not sure if it existed or we were aware of it when we implemented these methods. If we decide to use it, we may need to change the tests, so that we continue to compute the expected kinematic variables with an 'independent' method.

The simplest way would be to check the kinematic variables for a very simple motion case, for which the velocity, acceleration and displacement are very easy to infer / derive by hand. In a way this is similar to the tests we currently have, except that we can more easily infer which values are expected. I added an example test like that called test_kinematics_uniform_linear_motion for reference, but right now it takes only the valid bboxes dataset.

References

Overflow from #246

How has this PR been tested?

Tests pass locally and in CI.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

I updated the docstrings for the kinematics module so that the API reference is no longer specific to poses datasets.

Checklist:

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

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.77%. Comparing base (a98ff45) to head (fad3257). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #265 +/- ## ======================================= Coverage 99.77% 99.77% ======================================= Files 14 14 Lines 883 883 ======================================= Hits 881 881 Misses 2 2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lochhh commented 1 month ago

Cool! I wasn't aware of xr.differentiate(), which is much simpler in code for our use case:

result = data.differentiate("time")

vs.

result = xr.apply_ufunc(
               np.gradient,
               data,
               data.coords["time"].values,
               kwargs={"axis": 0},
         )
result = result.reindex_like(data)

And in both cases, we no longer need to assume equidistant time-spacing. (Opened #268 for this)

sfmig commented 1 month ago

thanks for the review @lochhh. A summary of the main changes below.

In the tests:

I added a fixture for a poses dataset with 2 individuals, each one with 3 keypoints, moving in a linear uniform motion.
The name of the added fixture is valid_poses_dataset_uniform_linear_motion, which doesn't really match the equivalent bboxes fixture but I didn't want to mess up existing tests that use valid_poses_dataset or valid_bboxes_dataset. I leave that to the refactoring fixtures PR (issue #222 ) if that is alright.
I added a test for datasets with nans.

In the docstrings of the kinematics functions:

I used monospace formatting for when we refer to arrays in a movement dataset.
I removed (x,y) from cartesian, as the functions should work just fine if the trajectory is in 3D (x,y,z).
I replaced some references to "vector" by the term "array" where possible, else I clarified the relationship between them (e.g., a position array is made up of position vectors across time). Let me know if this is clear enough.
I specified that the input position arrays should be in cartesian (even though this is not enforced in the implementation for now).
I added Notes to clarify what would happen when different position arrays are fed to each function. I am not sure if this is overkill / helpful, maybe we should remove it. I don't feel strongly either way.

Will re request review now