Add the option for multiple views loading

[vigji]

Description

This PR attempts at defining a loading mode for supporting multi-camera data.

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

This kind of data structure would be needed by anyone in the process of extracting 3D poses. This is something needed only as long as you do not have yet 3D coordinates, in my case (but I assume in anyone's) the end goal of having multiple views. So, if you want to support only the final, "clean" data where some triangulation of sort already happened, I would understand!

References

Discussion started in #196

How has this PR been tested?

Currently in the process of defining tests, atm just with already existing data, pretending it to be multi-camera.

Is this a breaking change?

If we decide to approve this, it will require some core redefinition in the dataset validator. Currently it seems to be quite strict; if the view dimension ends up being optional, it would require some work in the Validator class. I am not familiar with the processing functions to compute kinematics, in principle I assume the semantic indexing of xarray should allow for a smooth handling of this optionality - but I am not yet an xarray pro

Does this PR require an update to the documentation?

If accepted, yes.

If any features have changed, or have been added. Please explain how the
documentation has been updated.

No changes yet

Checklist:

• [ x] 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]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

[codecov]

Codecov Report

Attention: Patch coverage is 99.32318% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.34%. Comparing base (426003c) to head (8c6b213).
Report is 58 commits behind head on main.

Files with missing lines	Patch %	Lines
movement/io/load_poses.py	87.09%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   99.68%   99.34%   -0.35%     
==========================================
  Files          11       14       +3     
  Lines         638      912     +274     
==========================================
+ Hits          636      906     +270     
- Misses          2        6       +4     

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[niksirbi]

Cross-linking this thread on Zulip, where we are discussing matters relevant to this PR.

[niksirbi]

Hey @vigji, sorry for the long silence on this. I'm going through stale PRs and I think it's time to resurrect this idea.

For context, we are slowly undertaking changes to allow for more flexible dimensions in movement datasets, as you had suggested. We got rid of the accessor #322, and with it some overzealous validation steps. Now we only validate the dimensions we actually need, on a per-function basis. We are also planning to reorder the dimensions #236 and stop creating singleton dimensions, but these changes are still to be implemented.

Given the above, I think it's time to go forward with this PR. As far as I can tell, the extra "views" dimensions shouldn't prevent you from running filtering and kinematics functions on your datasets. The computations will be simply broadcast across all extra dimensions.

I think the first step would be to rebase this PR on the current main branch and check if the function or its docstring need updating. After that, it would just be a matter of modifying some tests - e.g creating a fixture with a multi-view dataset, and adding that to the parameters of some filtering and kinematics tests. I can do that last bit if you can't find the time. Let me know if you'd have time for this in the coming weeks and if you'll need help with anything.

[niksirbi]

Trying another ping, @vigji. I'm keen on your input.

[vigji]

Thanks for re-pinging me and keeping me in the loop!

I've lost a bit track of this but I started working back on it just this week, so I'll give this a try. So to confirm, now you think I can just rebase to main and the kinematics functions should work?

Also, is there anything holding #236 and dropping singletons back? Do I still have to work under the assumption that I need "individuals" dimensions? adding my arbitrary "view" dimension is not an issue at all in this context? It feels that until you're free to drop singletons you are not really in "time and space are the only requirements" mode yet, right?

[niksirbi]

Also, is there anything holding #236 and dropping singletons back? Do I still have to work under the assumption that I need "individuals" dimensions? adding my arbitrary "view" dimension is not an issue at all in this context? It feels that until you're free to drop singletons you are not really in "time and space are the only requirements" mode yet, right?

Afaik @lochhh is planning to tackle #236 soon, and nothing should be holding us back from dropping singletons (other than what unforeseen effects we may discover when we try it).

If you have a go at it now, assume that individuals is still needed. That said, your extra views dimensions should not prevent you from computing kinematics or applying filtering functions as things stand. That's because functions in filtering.py and kinematics.py only validate the dimensions they actually require for the computation (in most cases that's time, space or both), and they should simply broadcast over all other existing dimensions. So if you have multiple views, a given computation - e.g. compute_velocity() - will be performed on every view and the resulting DataArray will retain the views dimension. If you discover that something doesn't work as expected, then it's a bug and we'll have to solve it.

So in summary, I suggest implementing the multi-view loading assuming the current number and order of dimensions. When we fix #236 and the singleton problem, it will be our (the maintainers') job to make sure that your function is updated accordingly (together with all other loader functions). That's in the interest of keeping PRs atomic (at least conceptually).

Does that make sense?

[vigji]

Awesome! Yes, it does, I'll give this a try this week :)

[vigji]

So, I'm back to this! I can confirm that kinematics computation now runs smoothly, this is pretty cool :)

At the moment, there is a single function taking a dictionary {view_name: view_filename} . While I feel this is the most agnostic way of loading such data, it leaves upstream the burden of creating the dictionary. In my code I am currently using a function that looks for all .slp or .dlc files in a folder, and create the dictionary above using as keys the non-matching part of the filenames. I'd be happy to add it as well, depending on how you feel on having two functions both referring to a single use case and possibly a bit idiosyncratic.

I need to add sample data+test+miniml documentation. Can you add me to the GIN repo? (same username as here). My sample data would be 5 .dlc datasets corresponding to the 5 different views. I feel that it would be more correct to keep them all within a folder, although I do not see any other subfolders inside poses/ and I don't know if this would be a problem.

Let me know!

[vigji]

Sorry, I actually realised that in principle I can also test with what is already here, and it is what I was doing in the past. A separate folder with my data could help though if

1. I also include the function that I mentioned above
2. in the future, I port to this project also triangulation code - don't know if there could be interest in that

[niksirbi]

So, I'm back to this! I can confirm that kinematics computation now runs smoothly, this is pretty cool :)

Great to hear!

At the moment, there is a single function taking a dictionary {view_name: view_filename} . While I feel this is the most agnostic way of loading such data, it leaves upstream the burden of creating the dictionary.

I'm perfectly fine with that. I think that flexibility is desirable and goes well with our philosophy. We can't imagine everyone's use-cases, and most movement users probably will be able to put a dict together. That said, we might consider adding some more convenient/specialised entry points in the future, but for the first implementation your current approach is perfectly sensible.

I need to add sample data+test+minimal documentation. Can you add me to the GIN repo? (same username as here). My sample data would be 5 .dlc datasets corresponding to the 5 different views. I feel that it would be more correct to keep them all within a folder, although I do not see any other subfolders inside poses/ and I don't know if this would be a problem.

I've just added you to the GIN repo. Relevant contributing docs (which you've probably already read). Sometimes GIN gives us troubles. If your run into unexpected issues, look into this excellent guide that @sfmig has written.

Regarding where to place the data, our current logic dictates that all files containing "poses" go into the poses folder, without subfolders. In your case I would add some sensible suffix to indicate that they belong together - e.g. {name}_view-0.csv, {name}_view-1.csv etc.. Unfortunately, the whole logic of sample data is designed around the idea that each "poses" file is a dataset, so each one needs its own entry in the metadata.yaml file. Semantically, it would make sense to somehow group these files together. Perhaps this could be achieved via zipping them? But in that case additional logic would have to be added for pooch to unzip archives after downloading, which is a bit of an overhead. We'll have to look into the sample_data.py module and see if there is a "minimally invasive surgery" we can do to unlock your use-case. I'll check to see if there is an ease way without overhauling the entire thing, which we plan to do at some point, see #237.

Despite these pains, I'm keen to find a solution, because such data will be useful for you and us to develop and debug features related to triangulation (and associated stuff in napari).

Now some questions regarding your data:

• Are you adding just the predicted poses or associated videos/frames as well?
• How big are the files? We want to keep things small-ish to not slow down tests, CI, documentation builds etc.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[vigji]

So, I'd say: let's leave this PR and merge as it is now. We'll figure out a solution for the actual multi-view dataset in a separate PR dealing with triangulation, what do you think?

Off for the size, my poses data would be 5x77kb files of just 200 representative timepoints. I can have also the triangulated data - similar size - and the original videos, which are however small (4x(600,200) videos and 1x(600, 600) video, all 200 frames).

But again, this can be addressed in a separate PR!

[niksirbi]

So, I'd say: let's leave this PR and merge as it is now. We'll figure out a solution for the actual multi-view dataset in a separate PR dealing with triangulation, what do you think?

Agreed!

Off for the size, my poses data would be 5x77kb files of just 200 representative timepoints. I can have also the triangulated data - similar size - and the original videos, which are however small (4x(600,200) videos and 1x(600, 600) video, all 200 frames).

Perfectly acceptable sizes!

But again, this can be addressed in a separate PR!

Absolutely.

On an unrelated note, do we want to rebase this PR to main maybe, instead of merging? The diff is quite hard to parse as is.

[niksirbi]

@vigji I've invited you as a collaborator with write access to this GitHub repo. This way I won't have to keep approving the CI workflow runs and you can debug faster.

[vigji]

Yes sorry don't know what happened there, was trying to rebase and made a mess! Will close this and start over from main with just the relevant changes!