Compute pairwise distances

[lochhh]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?
Closes #277

What does this PR do?

• Adds functions to compute pairwise distances between elements (e.g. individuals, keypoints)
  • between a single pair of elements (_cdist - open to rename suggestions!)
  • between a single, multiple, or all pair(s) of elements (compute_pairwise_distances)
• Includes example usage in docstrings
• Adds tests for the new functions
  • with poses dataset (with single and multiple individuals/keypoints) or da.sel data with coord_val.ndim == 0
  • with bboxes dataset (with single and multiple individuals, but missing keypoints dim)

TO-DO:

• Handle poses dataset
  • with single individual/keypoint coord_val.ndim == 1
  • from da.sel with coord_val.ndim == 0
  • with multiple individuals/keypoints
• Handle datasets without "individuals" or "keypoints" dim, e.g. bboxes dataset doesn't have "keypoints" dim
• Revert de4a96b that works around failing tests due to sleap-io's incompatibility with NumPy 2.0.

References

#277

How has this PR been tested?

Tests passing locally and on CI

Does this PR require an update to the documentation?

Docstrings with examples are provided alongside the new functions.

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.77%. Comparing base (b10896f) to head (d1ca2cd).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #278   +/-   ##
=======================================
  Coverage   99.76%   99.77%           
=======================================
  Files          14       14           
  Lines         846      878   +32     
=======================================
+ Hits          844      876   +32     
  Misses          2        2           

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

[sonarqubecloud]

Quality Gate passed

Issues
0 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 for your work @lochhh!

The implementation is completely functional as is, but I think it's worth discussing the API design here.

I was thinking about this all day today, and it's not an easy problem to wrap one's head around, so as usual I resorted to drawing sketches 😉 .

Let's consider the following example:

Our current implementation exposes 3 public functions, and multiple ways of solving the above problem (i.e. getting the 3 distances of interest):

It took me a while to figure things out. Your examples were of great help and absolutely necessary, but in general it's not easy to figure out which function you need to use and there are multiple equivalent ways of doing things.

Here's what Id' propose as alternative:

We can debate the need to make cdist private or not, but in my eyes it doesn't add any extra functionality or convenience to just having compute_pairwise_distances.
You could argue that one can use cdist to compute distances across different datasets, but I can't foresee a use-case for this (I may be overlooking something).

[lochhh]

Thanks again @niksirbi for the usual thorough review! I've adopted your suggestion to remove the interkeypoint/individual wrappers, exposing only compute_pairwise_distances , whilst hiding _cdist - this makes sense as it's unlikely one would want to compute distances using 2 single-keypoint/individual data arrays from different datasets (with _cdist). Other changes include:

• requiring pairs as argument in compute_pairwise_distances and allowing the special keyword 'all'
• renaming core_dims → labels_dims to (hopefully) make it clearer that this is the dim to get labels from
• removing the full list of distance metrics, providing only a couple of examples in _cdist and compute_pairwise_distances docstrings
• raise ValueError when no pairs are found
• remove circularity while generating expected pairs in tests

[niksirbi]

Thanks @lochhh, I will take another look at this next week, with a fresh mind.

[niksirbi]

Thanks @lochhh, my remaining comments are mostly about some choices of phrase in the public docstring.

Other than these, the PR is good to go from my point of view. I'm not sure if @sfmig would like to also have a look before merging.

[sonarqubecloud]

Quality Gate passed

Issues
0 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

[sfmig]

sorry both for the delay and thanks for pinging me ❤️

I can have a look on Friday, but I know that would delay this work further, so feel free to go ahead! Maybe I can draft an example instead?

[lochhh]

sorry both for the delay and thanks for pinging me ❤️

I can have a look on Friday, but I know that would delay this work further, so feel free to go ahead! Maybe I can draft an example instead?

That would be perfect!

[niksirbi]

Thanks for the initiative @sfmig.
It's good to keep things moving fast.
Plus, if you discover a flaw with the implementation while writing the example, we can always modify the implementation.

Perhaps the DLC dataset with the two mice would be a good candidate for the example?