Skip to content

Conversation

@HARSHDIPSAHA
Copy link

@HARSHDIPSAHA HARSHDIPSAHA commented Jan 25, 2026

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

The _cdist function in movement/kinematics/distances.py had a semantic bug where it incorrectly assigned coordinates for the second element (elem2) in pairwise distance calculations. The function was using coordinates from array a for both elem1 and elem2, when elem2 should correctly use coordinates from array b.

While this bug may not cause functional issues in typical use cases (where both arrays come from the same dataset and have identical coordinates after xarray alignment), it represents a code correctness issue that should be fixed for:

  • Semantic correctness
  • Code maintainability
  • Future-proofing against edge cases
  • Clarity of intent

What does this PR do?

This PR fixes the coordinate assignment bug in _cdist by changing line 102 from:

elem2: getattr(a, labels_dim).values,  # Incorrect

to:

elem2: getattr(b, labels_dim).values,  # Correct

This ensures that the elem2 dimension is correctly assigned coordinates from array b's labels_dim, matching the semantic intent of the function where elem1 comes from a and elem2 comes from b.

References

How has this PR been tested?

  1. Existing tests pass: All existing unit tests for _cdist continue to pass, confirming that the fix doesn't break existing functionality:
    pytest tests/test_unit/test_kinematics/test_distances.py::test_cdist_with_known_values -v
    Both test cases (individuals and keypoints dimensions) pass successfully.

Is this a breaking change?

No

Does this PR require an update to the documentation?

No...

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (existing tests cover this functionality)
  • The documentation has been updated to reflect any changes (no documentation changes needed)
  • The code has been formatted with pre-commit (no formatting issues, linting passes)

@sonarqubecloud
Copy link

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.

1 participant