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

Add the BMTI method for density estimation #119

Closed AldoGl closed 4 months ago

AldoGl commented 4 months ago

Proposed changes

In this PR we:

Work done with @charliematteo.

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply

Checklist

Put an x in the boxes that apply.

codecov-commenter commented 4 months ago

Codecov Report

Attention: Patch coverage is 87.29097% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 81.61%. Comparing base (7a1f695) to head (c2a1a96).

Files Patch % Lines
dadapy/density_advanced.py 80.85% 27 Missing :warning:
dadapy/neigh_graph.py 90.43% 11 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #119 +/- ## ========================================== + Coverage 81.07% 81.61% +0.54% ========================================== Files 17 20 +3 Lines 3006 3285 +279 ========================================== + Hits 2437 2681 +244 - Misses 569 604 +35 ```

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

AldoGl commented 4 months ago

@charliematteo, I did what I planned. For some reason the test "test_density_BMTI()" runs perfectly on my machine, also using tox, but when it runs here it returns an error in the solution of the linear system. Very strange, it might be machine dependent. I can change the dataset tested and check that that is the problem.

AldoGl commented 4 months ago

Ok, I fixed that issue. Now we have the problem that python 3.7 gives different results for some routines, also a very strange one

AldoGl commented 4 months ago

Ok, I managed to fix that as well @charliematteo

AldoGl commented 4 months ago

I think this work is now ready to be merged. It would be useful if another developer had a quick look to check whether the logic of the new modules is clear. @diegodoimo you were potentially interested in the method, would you mind checking it out? (very rapidly, when you have time!)

diegodoimo commented 4 months ago

I'll do it in the weekend

AldoGl commented 4 months ago

Thank you @diegodoimo, and don't worry too much about it!

diegodoimo commented 4 months ago

A minor point: I do not understand why there is compute_density_BMTI which just calls compute_density_BMTI_reg hardcoding some kwargs. Could this be in a single function instead of having two? In the future, we should also consider unifying the two density classes in a single density interface. Anyway, it looks good and can be merged to me; great job!

AldoGl commented 4 months ago

Thank you very much @diegodoimo. The idea was to remove unnecessary arguments for the standard BMTI, but probably you are right and it's better to simply use sensible default values for unused arguments. I will implement this change. Thank you again!