MAINT additional cleaning in reachibility.pyx #5
Conversation
|
It was easier to make a PR than commenting. |
| a core point. | ||
| max_dist : float, default=0.0 | ||
| max_distance : float, default=0.0 |
There was a problem hiding this comment.
I think that we can be explicit regarding the naming.
| `LIL` format. | ||
| min_points : int, default=5 | ||
| min_samples : int, default=5 |
There was a problem hiding this comment.
Change the name to be consistent with the high-level function and DBSCAN as well
| )[farther_neighbor_idx] | ||
| else: | ||
| core_distances[i] = np.infty | ||
| core_distances[i] = INFINITY |
There was a problem hiding this comment.
This would avoid a Python interaction
| graph of a distance matrix. Note that computation is performed in-place for | ||
| `distance_matrix`. If out-of-place computation is required, pass a copy to | ||
| this function. | ||
| def mutual_reachability_graph( |
There was a problem hiding this comment.
Since we build the graph, I prefer to make it explicit.
|
The test could be something like: import numpy as np
from scipy import sparse
from sklearn.utils._testing import assert_allclose
dist = np.random.randn(5, 5)
mr_dense = mutual_reachability_graph(dist)
mr_sparse = mutual_reachability_graph(sparse.lil_matrix(dist))
assert_allclose(mr_sparse.A, mr_dense)With my PR it fails. I will have a closer look to see what is wrong. |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
I put the PR WIP but basically this is now working.
I have to update the documentation and make a pass to be sure that I like what I wrote.
| graph of a distance matrix. Note that computation is performed in-place for | ||
| `distance_matrix`. If out-of-place computation is required, pass a copy to | ||
| this function. | ||
| ctypedef fused integral: |
| return distance_matrix | ||
| core_distances[i] = INFINITY | ||
|
|
||
| for col_ind in range(n_samples): |
|
I am pretty happy with the current implementation.
A bit later, I will add a file that tests minimally the implementation. Something that I was wondering and seems quite important, it seems that we assume that the distance used is symmetric. Otherwise, the way we build the |
2 participants
Some additional style change