Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add from_mmp_file function #198

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Add from_mmp_file function #198

wants to merge 6 commits into from

Conversation

ivanvrlg
Copy link
Collaborator

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

This is the first step I have taken towards including I/O functionality for MMPose, as we discussed. The PR is unfinished as no tests have been written yet, I am mainly seeking feedback.

What does this PR do?

I have included a function from_mmp_file, which loads the data from the JSON file generated by the MMPose inferencer. I am mainly seeking feedback as it is a long function and might best be contributed by breaking it down into several (maybe loading as pandas df and then converting to xarray).

References

#175

How has this PR been tested?

It has not! Any feedback on how to properly write comprehensive tests for the movement package is welcome.

Is this a breaking change?

N/A

Does this PR require an update to the documentation?

Documentation should be updated to reflect the new function allowing for user input of MMPose Json files. I have included a lengthy docstring with the intention that it will explain functionality and expected input.

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

@ivanvrlg ivanvrlg requested review from sfmig and niksirbi May 30, 2024 11:51
@ivanvrlg ivanvrlg marked this pull request as ready for review May 30, 2024 11:52
@ivanvrlg ivanvrlg closed this May 30, 2024
@ivanvrlg ivanvrlg reopened this May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 7.40741% with 25 lines in your changes missing coverage. Please review.

Project coverage is 95.91%. Comparing base (53a9eff) to head (f4983d7).

Files Patch % Lines
movement/io/load_poses.py 7.40% 25 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
- Coverage   99.68%   95.91%   -3.77%     
==========================================
  Files          11       11              
  Lines         634      661      +27     
==========================================
+ Hits          632      634       +2     
- Misses          2       27      +25     

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

@ivanvrlg ivanvrlg marked this pull request as draft May 30, 2024 11:53
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.

That's a great start @ivanvrig.
This PR has now been rebased to main, to bring it up to date with the refactored IO modules (see PR #194). I've left some comments which mostly have to do with aligning your function with the changes made in that PR.

Other, than that, here's a TODO list from our discussion today:

  • add a valid MMPose .json file to our data repository, following the relevant instructions.
  • use the above sample data to implement some unit tests for the new function (at least test what happens when loading a valid vs an invalid file).
  • you may leave the json validator for a future PR. In fact I think you should, as it's not absolutely essential for this loader to function.

movement/io/load_poses.py Outdated Show resolved Hide resolved
movement/io/load_poses.py Outdated Show resolved Hide resolved
Comment on lines +690 to +704
[
{
"frame_id": int,
"instances": [
{
"keypoints": list[list[float]],
"keypoint_scores": list[float],
"bbox": list[float],
"bbox_score": float
},
...
]
},
...
]
Copy link
Member

Choose a reason for hiding this comment

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

This infor is useful, but better move it to the "Notes" section of the docstring, or better still add a reference to the corresponding MMPose documentation page where the format is describe. See from_sleap_file() for the syntax for "Notes" and "References".

movement/io/load_poses.py Outdated Show resolved Hide resolved
Comment on lines 763 to 772
valid_data = ValidPosesDataset(
position_array=tracks_array,
confidence_array=scores_array,
individual_names=[
f"individual_{i}" for i in range(tracks_array.shape[1])
],
keypoint_names=keypoint_names,
fps=fps,
)
ds = _from_valid_data(valid_data)
Copy link
Member

Choose a reason for hiding this comment

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

use the new from_numpy() function instead.

ds = _from_valid_data(valid_data)

# Metadata
ds.attrs["source_software"] = "JSON"
Copy link
Member

@niksirbi niksirbi Jun 4, 2024

Choose a reason for hiding this comment

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

I would call this "MMPose", as that is the software. json is just the file format and may be used by other software too.

Copy link

sonarqubecloud bot commented Jun 4, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@sfmig sfmig linked an issue Aug 27, 2024 that may be closed by this pull request
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 I/O support for MMPose
2 participants