sissa-data-science / DADApy

Distance-based Analysis of DAta-manifolds in python
https://dadapy.readthedocs.io/
Apache License 2.0
99 stars 16 forks source link

Fixed cross NN with periodic box and added tests. #102

Closed ollyfutur closed 8 months ago

ollyfutur commented 8 months ago

Proposed changes

Correction of a bug in utils.compute_cross_nn_distances. When using periodic conditions the function computed the self NN distances of dataset X with compute_NN_PBC instead of cross NN distances between X_new and X. This affected interpolated density estimation.

Types of changes

The cross NN distance is directly computed using cKDTree instead of calling compute_NN_PBC. I have added two tests in test_distances_utils, test_cross_nn_distances and test_cross_nn_distances_periodic.

AldoGl commented 8 months ago

Thank you @ollyfutur, this seems excellent work!

To me this seems immediately mergeable, but let's see what @wildromi or @imacocco say since they are the people who used periodic distances the most in the past.

codecov-commenter commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (d82322b) 80.29% compared to head (492fa30) 80.22%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #102 +/- ## ========================================== - Coverage 80.29% 80.22% -0.08% ========================================== Files 15 15 Lines 2431 2432 +1 ========================================== - Hits 1952 1951 -1 - Misses 479 481 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

imacocco commented 8 months ago

Everything looks fine, the tests also run smoothly. It would be great also to fix the linting, then we can merge straightforwardly!! Many thanks for the correction!

ollyfutur commented 8 months ago

Just fixed the linting, thanks for maintaining this great package :)

AldoGl commented 8 months ago

Thank you @ollyfutur for entering the team of contributors! 🙂 Since tests pass and @imacocco also checked the code, I will soon rebase and merge this PR 👍🏼

AldoGl commented 8 months ago

@ollyfutur if it's ok for you I will add you to the list of contributors here

ollyfutur commented 8 months ago

Alright thank you very much @AldoGl !

wildromi commented 8 months ago

Thank you Olivier @ollyfutur for spotting this bug!