Load bboxes dataset from VIA tracks file (3/4)

[sfmig]

Rebase after #234

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?
To be able to load a VIA tracks file with bounding boxes into a movement dataset.

What does this PR do?

• Adds a movement.io.load_bboxes module, which follows the equivalent poses one as much as possible.
• Adds corresponding unit tests in tests_load_bboxes.

Question: how to make mypy aware of the type transformations that take place in __attrs_post_init__?
For example, the confidence array passed to ValidBboxesDataset can be None, and mypy flags that a .shape attribute is used later which None doesn't have. But mypy seems to be missing that the confidence array is populated with nans in __attrs_post_init__ if None is passed as input. Is there a nice way to fix this?

References

This PR would close #167

How has this PR been tested?

Tests pass locally and on CI.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

I updated api_index.rst.

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.76%. Comparing base (d10ec20) to head (7c260b3).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #229      +/-   ##
==========================================
+ Coverage   99.74%   99.76%   +0.02%     
==========================================
  Files          13       14       +1     
  Lines         771      854      +83     
==========================================
+ Hits          769      852      +83     
  Misses          2        2              

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

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[niksirbi]

Thanks a lot @sfmig and well done on closely following the code architecture for loading poses. This makes me hopeful that we will be able to refactor them in future, such that we abstract away there common bits (to reduce repetition in validators and tests between poses and bboxes). But this shouldn't worry us right now.

I have two substantial comments apart form the cosmetic/trivial issues I've highlighted in specific comments.

• I don't fully understand what frame_array is needed for and how it is handled. See my specific comment about this. Perhaps we can discuss it in person together?
• The documentaiton pages for input_output.md and movement_dataset.md need to be updated. Specifically:
  • the new format has to be aded in the "Supported formats" section of Input/Output.
  • A new "Loading tracks of bounding boxes" section has to be added to Input/Output.
  • The "Movement Dataset" page has to also include information about the bboxes dataset format, especially where it differs from the poses format

I'm happy for you to leave the documentation changes for a future PR (just open an issue in that case).

EDIT

Another thought that just came to mind: have you tested whether the existing filtering and kinematics functions work on the new bboxes datasets?

You could define a valid_bboxes_dataset pytest fixture (similar to valid_poses_dataset) and add it to all relevant tests in:

• test_integration/test_filtering.py
• test_unit/test_filtering.py
• test_unit/test_kinematics.py
• test_unit/test_move_accessor.py

It's crucial to determine if our existing features work on the new type of dataset.

[sfmig]

From chats with @niksirbi:
We agreed to take the frame numbers as specified in the csv file for now (that is, not necessarily starting from 0), but to make the time coordinate have the same origin as the frame number.

[sfmig]

Thanks @niksirbi for the feedback!

I think I addressed all the comments, let me know if something is missing.

frame_array and frame numbers

I did some changes to hopefully make more understandable the behaviour of keeping / resetting the frame numbers:

• In all the loaders of bboxes data (from_file, from_via_tracks_file, from numpy), the default behaviour is that the origin of the time dimension is the first loaded (aka tracked) frame, which would be frame number 0 captured at t = 0 seconds. This is to stay consistent with the pose data, and it is likely the most natural for our users.

• In the from_file and the from_via_tracks_file I added a use_frame_numbers_from_file option, that is False by default. If True, it will take the frame numbers from the input file, with whatever assumption for the time origin they have (they may assume for example the first frame is 0, or 1).

  • The caveat is that if you take the frame number from the file, and later transform it to seconds, then the time origin is assumed to be at frame number 0 and time = 0 seconds. This is because we simply transform to seconds as frame_number/fps.
  • Another option could be to add an extra attribute / argument to set the time origin manually, but I didn't want to complicate it too much. Let me know if it is clear as is, if not I am happy to give it another go.
  • If users want to keep track of the frame relative to the full video at which the analysed clip starts, they could assign a custom attribute. I sneakily added this in PR Getting started docs update for bboxes #245 - let me know thoughts.

• For the from_numpy loader, this is simply done via the frame_array input. If passed, those frame numbers are used, but the input is None by default, which means the frame numbers are assigned from 0 to N-1, with N being the first dimension of the position array.

Update Getting started documentation

• I moved this to a separate PR Getting started docs update for bboxes #245

Extend tests to ensure postprocessing methods work with bboxes datasets

• Working on it in this (currently draft) PR Extend tests for bboxes datasets #246

Question

How do you think we should merge these PRs?
I have never done this but I'm wondering if we should merge PR #245 and #246 into this PR's branch, before merging this into main. Or should we use a dev branch?

[niksirbi]

Thanks @sfmig, I'm happy with how you've handled things, and the updated docstrings make it super clear what happens to time and frames.

I've only left very few suggestions re docstrings, feel free to adopt or not.

[niksirbi]

How do you think we should merge these PRs?
I have never done this but I'm wondering if we should merge PR #245 and #246 into this PR's branch, before merging this into main. Or should we use a dev branch?

I've never done this as well. Maybe we should consult with Alessandro or Joe who have more experience with complex merges.
How about merging each of these 3 PRs separately (and sequentially) to main (with rebases in-between)? Is that not an option? It would be nice to have them as 3 separate commits in main, but if that' painful, feel free to merge any way that's convenient.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud