[MRG+1] OPTICS remove redundant recursion #11985
Conversation
69299ea to
e414965
Compare
|
@espg, I'd really like your review on this one. |
|
I'm less certain on this and what the block of code does as this section is mostly @amyxzhang 's code. But, it's possible that there may be some loss of function related to hierarchy extraction? I know that the original code let you pick out which level of the hierarchy from the returned root node of the tree (i.e., converting to a dendrogram). I wasn't sure how the implement any of that with the sklearn API, so it's not implemented...but there could be a follow along PR that addresses that capability. |
|
I also don't see why the original code is doing it, I may be missing something though. I've asked the question on the original repo (amyxzhang/OPTICS-Automatic-Clustering#3). |
|
I can't remember why I did this but your logic is sound, so I agree with it being removed. |
|
Awesome, thanks @amyxzhang! |
See #11857 (comment)
This recursion will continue on
local_maxima_points, which is sorted by theirreachability_. Therefore all the following recursions will also fall into this condition. As a result it's redundant and not necessary to continue the recursion at the point where it falls into this condition.