-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
DOC Linked examples for clustering algorithms in their docstrings (#26927) #30127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOC Linked examples for clustering algorithms in their docstrings (#26927) #30127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SuccessMoses! Thank you for your PR.
Could you move the references to the end of the respective "Examples" sections in the docstrings of the estimators? (We haven't been consistent with placing the references in the past and are trying to be more consistent now. 🙂 )
Could you also reference this example in the same way in the docstrings of
MeanShift, MiniBatchKMeans, AgglomerativeClustering, AgglomerativeClustering, Birch and GaussianMixture?
I am working on it |
Could you also make a new rubric "Examples" at the end of the section here and link the example there (compare other PRs if needed). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small comments. Otherwise it looks good.
doc/modules/clustering.rst
Outdated
.. rubric:: Examples | ||
|
||
* :ref:`sphx_glr_auto_examples_cluster_plot_cluster_comparison.py`: Shows the | ||
characteristics of different clustering algorithms of 2D datasets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick: could you replace "of 2D datasets" with "on 2D datasets"?
sklearn/cluster/_birch.py
Outdated
@@ -483,6 +483,9 @@ class Birch( | |||
Birch(n_clusters=None) | |||
>>> brc.predict(X) | |||
array([0, 0, 0, 1, 1, 1]) | |||
|
|||
For a comparison of BIRCH clustering algorithm with other clustering algorithms, see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nitpick: could you write "of the BIRCH .."
doc/modules/clustering.rst
Outdated
@@ -140,6 +140,11 @@ model with equal covariance per component. | |||
:term:`inductive` clustering methods) are not designed to be applied to new, | |||
unseen data. | |||
|
|||
.. rubric:: Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrinjalali are you ok with having the example linked here? As noted before it's not obvious that it's possible to click on the images and access examples that way. Also, we haven't been consistent with linking examples in the user guide. There are quite a few places in the user guide where a link to an example is listed under the rubric "Examples" even though you can access the respective example through clicking on the image in that section (such as here: https://github.com/scikit-learn/scikit-learn/blob/main/doc/modules/clustering.rst?plain=1#L306)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair question. I'm not sure what I think about the question in general, but in this particular case, I think this is kinda repeating the same information right after where it's mentioned, and hence unnecessary.
However, your point on the images not being clearly clickable is valid, and we can probably improve the image caption to mention they can be clicked?
I'm not sure, maybe @Charlie-XIAO or @glemaitre have a better idea of how to improve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think the link is redundant, there's a nice description with a hyperlink above here.
However, it's a good point that it's not clear the the user can click on them. I wonder if @Charlie-XIAO would have an idea of how we could improve that UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SuccessMoses! Could you remove this link including the "Examples" rubric you made? We're then ready to merge. I'll open an issue for discussing how to best improve the UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
doc/modules/clustering.rst
Outdated
@@ -140,6 +140,11 @@ model with equal covariance per component. | |||
:term:`inductive` clustering methods) are not designed to be applied to new, | |||
unseen data. | |||
|
|||
.. rubric:: Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair question. I'm not sure what I think about the question in general, but in this particular case, I think this is kinda repeating the same information right after where it's mentioned, and hence unnecessary.
However, your point on the images not being clearly clickable is valid, and we can probably improve the image caption to mention they can be clicked?
I'm not sure, maybe @Charlie-XIAO or @glemaitre have a better idea of how to improve this.
doc/modules/clustering.rst
Outdated
@@ -140,6 +140,11 @@ model with equal covariance per component. | |||
:term:`inductive` clustering methods) are not designed to be applied to new, | |||
unseen data. | |||
|
|||
.. rubric:: Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think the link is redundant, there's a nice description with a hyperlink above here.
However, it's a good point that it's not clear the the user can click on them. I wonder if @Charlie-XIAO would have an idea of how we could improve that UX.
Hi @SuccessMoses, I have just seen that meanwhile a few PRs have been merged that are touching on what you are doing here:
As you go forward with this PR, please consider to align this. |
The best way to align with the other PRs is to pull the changes from upstream main into your feature branch like so:
|
I finished this PR because it was blocking another PR from getting merged. Thank you for your contribution @SuccessMoses! |
Reference Issues/PRs
Adds links to examples/cluster mentioned in #26927
What does this implement/fix? Explain your changes.
Adds links to auto-generated examples for classes
AffinityPropagation
,SpectralClustering
,DBSCAN
,HDBSCAN
andOPTICS
. The example shows comparison between different clustering methods.Any other comments?
Examples linked:
plot_cluster_comparison.py