[MRG] MNT rename min_cluster_size_ratio to min_cluster_size #11913
Conversation
sklearn/cluster/optics_.py
Outdated
| neighborhood_size = int(min_maxima_ratio * len(ordering)) | ||
|
|
||
| # Should this check for < min_samples? Should this be public? |
There was a problem hiding this comment.
I don't think it should be min_samples. The minimum cluster size in dbscan is less than min_samples. But I don't see why it should be 5 either.
There was a problem hiding this comment.
I have suggested minimum value of 2 in my PR, and changing the name to min_neighbors or something.
There was a problem hiding this comment.
Yes, I'm accepting your min cluster size. Not so much the min_samples name. See there.
|
@jnothman it makes sense. I like the change. |
sklearn/cluster/optics_.py
Outdated
| neighborhood_size = int(min_maxima_ratio * len(ordering)) | ||
|
|
||
| # Should this check for < min_samples? Should this be public? |
There was a problem hiding this comment.
Yes, I'm accepting your min cluster size. Not so much the min_samples name. See there.
|
This should be merged for release. Seeking reviews. |
sklearn/cluster/optics_.py
Outdated
| @@ -221,8 +223,10 @@ class OPTICS(BaseEstimator, ClusterMixin): | |||
| significant_min : float, optional | |||
| Sets a lower threshold on how small a significant maxima can be. | |||
|
|
|||
| min_cluster_size_ratio : float, optional | |||
| Minimum percentage of dataset expected for cluster membership. | |||
| min_cluster_size : int > 1 or loat between 0 and 1 | |||
sklearn/cluster/optics_.py
Outdated
| @@ -528,8 +532,10 @@ def _extract_optics(ordering, reachability, maxima_ratio=.75, | |||
| significant_min : float, optional | |||
| Sets a lower threshold on how small a significant maxima can be. | |||
|
|
|||
| min_cluster_size_ratio : float, optional | |||
| Minimum percentage of dataset expected for cluster membership. | |||
| min_cluster_size : int > 1 or loat between 0 and 1 | |||
|
Thanks @albertcthomas! Does the implementation appear correct? Does the naming seem to be improved by this change? |
| neighborhood_size = int(min_maxima_ratio * len(ordering)) | ||
|
|
||
| # Should this check for < min_samples? Should this be public? |
There was a problem hiding this comment.
Is there a specific reason for removing this in this PR?
There was a problem hiding this comment.
Well, I'm changing 5 to 2 above (using max instead of if). @adrinjalali and I seem to agree that min_samples is not the relevant criterion here.
There was a problem hiding this comment.
The role of min_samples is kinda different in OPTICS than DBSCAN. In OPTICS you can have the generator epsilon set to a large number (it's inf by default), to generate a smooth reachability_ array. Similarly, min_samples can also be set relatively large. Still, since the clusters are detected hierarchically, it can be the case that a cluster is smaller than the value for min_samples, and that's fine. I agree that it doesn't make much sense probably to do so, but they're independent parameters IMO.
|
I think we need some input validation (maybe some tests) here. Otherwise this LGTM. |
| raise ValueError('min_cluster_size must be a positive integer or ' | ||
| 'a float between 0 and 1. Got %r' % | ||
| self.min_cluster_size) | ||
| elif self.min_cluster_size > n_samples: |
There was a problem hiding this comment.
I assume that min_cluster_size == n_samples is possible if all the samples are in the same unique cluster
|
There's a |
Agreed @adrinjalali? Any objections @espg?