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

Loading function for Anipose data #358

Merged
merged 46 commits into from
Dec 11, 2024
Merged

Loading function for Anipose data #358

merged 46 commits into from
Dec 11, 2024

Conversation

vigji
Copy link
Collaborator

@vigji vigji commented Dec 6, 2024

Description

Adding support to load adipose-triangulated data.

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
Currently, adipose-triangulated data cannot be loaded. I love the autocorrect here.

What does this PR do?
Implement simple loading function for anipose-generated data. Anipose saves a csv file with x, y, z, score (confidence) plus more info to a csv file. Currently, this basic info is parsed. Additional info that is stored in the file, which could be loaded in principle but would not fit any of movement boxes atm:

  • number of cameras each key point was triangulated from. This is a useful piece of data; if there's space for arbitrary arrays in the dataset I would add it
  • back projection error: another important metrics that would be useful to pack together.
  • info on the coordinate transformations implemented during triangulation, less important imho - but could also be dumped in. Not sure if we would end up to bloat the DataSet too much

References

#124

How has this PR been tested?

So far just very coarsely tested on some anipose data I am generating. I am no proficient anipose user, and I'm happy to collect example data and feedback from anyone. Will try to dig into example data from the anipose repo to make sure everything can be loaded from there.

Is this a breaking change?

No

Does this PR require an update to the documentation?

Maybe once done

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

@vigji vigji marked this pull request as draft December 6, 2024 14:22
@adamltyson
Copy link
Member

I love the autocorrect here.

Thanks for adding support for fat data.

@niksirbi
Copy link
Member

niksirbi commented Dec 6, 2024

Thanks for opening this PR @vigji, I will take a look at it next week, and also think about the possibility of storing the additional data as arrays in the same dataset.

Off the top of my head we might take a 2-step-approach:

  • in this PR, just load x, y, z + confidence (all of which are supported by current data structures).
  • add the extra arrays in a future PR, after discussing how to represent those variables

Also, following this PR, and before it's released, we'd have to go through our functions (filtering, kinematics, vector utils) and make sure they don't freak out with 3D data. Many of them will be fine, but some of them have not been designed/tested with 3D in mind. But this is a bullet we have to bite anyway sooner or later, and sooner would be much easier.

@vigji
Copy link
Collaborator Author

vigji commented Dec 6, 2024

Makes perfect sense! I will deal with anipose loading + test kinematic stuff on 3D model here, and leave additional datasets loading for another PR.

I could dig out some open source anipose data from the Anipose publication, what is your policy there? Should I just copy it to the GIN storage, ask first, or?

@niksirbi
Copy link
Member

niksirbi commented Dec 9, 2024

I could dig out some open source anipose data from the Anipose publication, what is your policy there? Should I just copy it to the GIN storage, ask first, or?

With our current infrastructure, the easiest would be to just upload the data to GIN, if the license of the original data allows that. Push access to our GIN repo is restricted, but you should already have the required permission.

I'd just make sure to also include the licensing info in the README, like we've done for the MOCA crabe dataset here.

Long term, we are also considering to add support for downloading publicly available datasets, as long as they have persistent identifiers and reliable URLs, but this is not implemented yet. When we have that, it may no longer be necessary to mirror everything on GIN.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.79%. Comparing base (1387cc1) to head (bbb5ab9).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   99.78%   99.79%           
=======================================
  Files          14       14           
  Lines         932      969   +37     
=======================================
+ Hits          930      967   +37     
  Misses          2        2           

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

@vigji vigji requested a review from niksirbi December 10, 2024 07:06
@vigji
Copy link
Collaborator Author

vigji commented Dec 10, 2024

Dedicated validator and testing should have been implemented, let me know what you think and if you think docs should also be updated!

Kinematics remain to be tested.
From what I see in the kinematic functions

  • there should not be 2D specific stuff in velocity computing;
  • there is a check for 2 spatial dimensions in the compute_forward_vector; how do we want to deal with this? should we generalise the forward vector to 3D? Can that be done in a separate PR, and for now we just assume that for 3D data there is still a plane over which to project to compute those metrics? In my case, the mouse is still moving on a plane, so this is well defined. This might not be the case for swimming/flying/complex environments. But maybe this can be addressed separately, also with the contribution of people who deal with those kind of data, I do not want to define the framework for data I do not work with!

@niksirbi niksirbi linked an issue Dec 10, 2024 that may be closed by this pull request
@niksirbi niksirbi added the enhancement New optional feature label Dec 10, 2024
@vigji vigji marked this pull request as ready for review December 10, 2024 20:07
@vigji vigji requested a review from niksirbi December 10, 2024 20:07
@vigji
Copy link
Collaborator Author

vigji commented Dec 10, 2024

ok, this should be good now!

@vigji vigji dismissed niksirbi’s stale review December 10, 2024 20:29

Implemented all suggestions

movement/io/load_poses.py Outdated Show resolved Hide resolved
movement/io/load_poses.py Outdated Show resolved Hide resolved
movement/io/load_poses.py Outdated Show resolved Hide resolved
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.

Thanks for updating @vigji.

I've added a few comments as finishing touches.

This PR is still missing the updates on the docs input/output page.

movement/io/load_poses.py Outdated Show resolved Hide resolved
movement/io/load_poses.py Outdated Show resolved Hide resolved
movement/io/load_poses.py Show resolved Hide resolved
@vigji
Copy link
Collaborator Author

vigji commented Dec 11, 2024

Right, here we go! There should be everything now.

@niksirbi
Copy link
Member

Right, here we go! There should be everything now.

Very good, the anipose url had not been defined, but should be fine now, I've added it to must_url_schemes in Sphinx's conf.py. I will merge this once all CI checks have passed.

Thanks for doing all the work!

@niksirbi niksirbi added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit d190ce5 Dec 11, 2024
28 checks passed
@niksirbi niksirbi deleted the anipose-loader branch December 11, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New optional feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Load 3D pose data from Anipose
3 participants