[MRG+1] OPTICS correctly handle multiple infs in reachability array.#12029
[MRG+1] OPTICS correctly handle multiple infs in reachability array.#12029qinhanmin2014 merged 5 commits intoscikit-learn:masterfrom
Conversation
|
Please credit yourself for the OPTICS feature in what's new v0.20 |
|
I'm not sure why the python 2 ci is suddenly failing |
|
This seems ready to me. A second review is appreciated. |
sklearn/cluster/optics_.py
Outdated
|
|
||
| # Extraction wrapper | ||
| reachability = reachability / np.max(reachability[1:]) | ||
| # according to the paper (p. 5), for a small enough generative distance |
There was a problem hiding this comment.
Which paper? Maybe at least provide the author and the time.
|
|
||
| if ((avg_reach1 / reachability_plot[s]) > maxima_ratio or | ||
| (avg_reach2 / reachability_plot[s]) > maxima_ratio): | ||
| if ((avg_reach1 / maxima_ratio) > reachability_plot[s] or |
There was a problem hiding this comment.
(Not related) I'm wondering whether we need both maxima_ratio and rejection_ratio. Too many parameters make our OPTICS somehow unfriendly at least to me.
There was a problem hiding this comment.
If I understand it correctly, setting the two parameters the same, would result in each split point to accept only one of the children, since the next condition is exactly the opposite. But I agree this method has quite a few parameters, which can be confusing specially if the user hasn't read both the papers for OPTICS and this method (which we're calling SQLNK I guess).
There was a problem hiding this comment.
But I agree this method has quite a few parameters, which can be confusing specially if the user hasn't read both the papers for OPTICS and this method (which we're calling SQLNK I guess).
You find these two parameters in a paper? Which one? I only find one.
if ((average reachability value in any node in NL) / s.r_dist > 0.75)
// if split point s is not significant, ignore s and continue
cluster_tree(N, parent_of_N, L); //
There was a problem hiding this comment.
Apart from these two parameters, some other parameters also seems strange, e.g., significant_min (we're normalizing RD so I don't think it makes sense to users) and the magical check_ratio inside the implementation. I notice that amyxzhang noted that this is "An implementation of the following algorithm, with some minor add-ons". I think we need to check these add-ons carefully. As I missing something @adrinjalali, otherwise I'm going to open an issue. Thanks in advance.
qinhanmin2014
left a comment
There was a problem hiding this comment.
LGTM, thanks @adrinjalali
See #11857
The extraction part of the code assumes only the first
reachability_to beinf, but the array may include multipleinfs. This PR fixes the issue, and fixes a test which used to result in allinfvalues. It also adds aValueErrorin case all reachability values are inf. And also tests for thatValueError.This is yet another PR to extract parts of #11857.
Also see #11857 (review)
Ping @jnothman