better description of the hierarchical clustering parameter #9171
Conversation
rgommers
added
scipy.cluster
Documentation
|
the reasons pypy3 failed with PR seems to be for reasons completely unrelated to my changes - any idea whats going on? |
scipy/cluster/hierarchy.py
Outdated
| @@ -2471,8 +2471,11 @@ def fcluster(Z, t, criterion='inconsistent', depth=2, R=None, monocrit=None): | |||
| Z : ndarray | |||
| The hierarchical clustering encoded with the matrix returned | |||
| by the `linkage` function. | |||
| t : float | |||
| The threshold to apply when forming flat clusters. | |||
| t : float or int | |||
There was a problem hiding this comment.
This could really be any numeric that can be safely cast to int and float , correct? A boolean would work here as well. I like your new comment below, but am unsure if changing to "float or int" is an improvement.
There was a problem hiding this comment.
its more of a semantic hint to further reinforce the point that it is referring to number of clusters, an integer.. Making it explicit, speaking in python terms!. Without that hint, people may supply 2.0, thinking they are expected a float.
in fact, it might even be better to rename to t to t_or_num_clust
There was a problem hiding this comment.
in other places referring to kmeans, scipy already uses k_or_guess
There was a problem hiding this comment.
the generic term to use for float or int is scalar
There was a problem hiding this comment.
I take your point @raamana, I think scalar is the right way to go
There was a problem hiding this comment.
sure - what about the renaming the variable to t_or_num_clust?
There was a problem hiding this comment.
you cannot rename variables in the signature, that breaks backwards compatibility
There was a problem hiding this comment.
Sure. revised the docs now.
|
Merged, thanks @raamana, @jeffyancey |
|
Yay! thanks. So glad to be able to add a few characters into the mighty |
Clarifies that
tcould be an integer specifying the max number of clusters undermaxclust*criteria