[MRG][DOC] Fix inconsistencies in clustering doc. #13946
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jnothman
reviewed
May 26, 2019
jnothman
reviewed
May 27, 2019
|
Thanks @jnothman for your review. What about the following questions about documentation conventions ?
Currently it is not consistent between AffinityPropagation, DBSCAN, and AgglemerativeClustering. Do you think it should be updated for more consistency or remain as it is ? |
|
I'm okay with it being updated for consistency, but am fairly ambivalent to
which choice. I suppose "labels" is appropriate. "Index of the cluster"
doesn't make sense unless you have something to index into. That makes
sense for kmeans, but not dbscan where the labelling is arbitrary except
when it is -1. This description should probably be the same as that of
labels_.
And fit's return type should be the name of the current class ...
|
|
But often for fit's return value, we just write "self". No type needed
|
|
Thanks for your feedback. I have updated the documentation of I have noticed that there are the same inconsistencies in the documentation of other clustering algorithms ( |
This commit concerns only the clustering algorithms that can take as input affinity or distance matrices. * fit and fit_predict have now the same descriptions for their input arguments X and y. * The commit follows the documentation conventions detailed in issue scikit-learn#3791. * Precise the preferred sparse matrix format if relevant. * AgglomerativeClustering. The documentation of the fit method now clarifies that a distance matrix is expected (not an affinity / similarity matrix). The parameter affinity is misleading (see issue scikit-learn#13945).
Only the documentation is updated. Updating the name of the ̀ affinity` parameter would involve an API change, with a proper deprecation cycle.
No argument sample_weight.
No argument sample_weight.
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Description: "Cluster labels."
|
thanks! |
koenvandevelde
pushed a commit
to koenvandevelde/scikit-learn
that referenced
this pull request
Jul 12, 2019
* Fix inconsistencies in clustering doc. This commit concerns only the clustering algorithms that can take as input affinity or distance matrices. * fit and fit_predict have now the same descriptions for their input arguments X and y. * The commit follows the documentation conventions detailed in issue scikit-learn#3791. * Precise the preferred sparse matrix format if relevant. * AgglomerativeClustering. The documentation of the fit method now clarifies that a distance matrix is expected (not an affinity / similarity matrix). The parameter affinity is misleading (see issue scikit-learn#13945). * Answer partly to issue scikit-learn#13945. Only the documentation is updated. Updating the name of the ̀ affinity` parameter would involve an API change, with a proper deprecation cycle. * Wrong arguments for fit_predict of AffinityPropag. No argument sample_weight. * Wrong arguments for fit_predict of AgglomerativeCl No argument sample_weight. * Add a comma (review by jnohman) Co-Authored-By: Joel Nothman <joel.nothman@gmail.com> * Rm modification (review by jnothman) * Add commas. * Update sklearn/cluster/affinity_propagation_.py Co-Authored-By: Joel Nothman <joel.nothman@gmail.com> * Fix back quotes (review by jnothman) * add commas * fit_predict and predict return labels. Description: "Cluster labels." * Doc fit: returns "self". * Same modifications for SpectralClustering.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this implement/fix? Explain your changes.
This pull request concerns only the clustering algorithms that can take as input
affinity or distance matrices. It could be extended to the other clustering algorithms.
arguments X and y.
AgglomerativeClustering. The parameter affinity is misleading (see issue [AgglomerativeClustering] confusing parameter 'affinity' #13945).The documentation of the fit method now clarifies that a distance matrix is expected (not an affinity / similarity matrix).
A few questions about documentation conventions
fitmethods should indicate thatselfis returned ? In this case, what is the returned type ?object?predictof clustering algorithms ?yorlabels? What bout the description ? "Cluster labels" or "Index of the cluster each sample belongs to." ?