Skip to content

Conversation

siavrez
Copy link
Contributor

@siavrez siavrez commented Nov 21, 2021

Changing matplotlib.text with matplotlib.scatter and Markers created from TeX symbols improves runtime by almost 15 seconds.
Also changed time.time with time.perf_counter.

Reference Issues/PRs

#21598

What does this implement/fix? Explain your changes.

Changing matplotlib.text with matplotlib.scatter and Markers created from TeX symbols improves runtime by almost 15 seconds.
Also changed time.time with time.perf_counter.

Any other comments?

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

@@ -2,35 +2,28 @@
=============================================================================
Various Agglomerative Clustering on a 2D embedding of digits
=============================================================================

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert all the changes in the first docstring. They are not linked with the purpose of the PR and this is fine to have paragraph.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge once there are reverted

"""

# Authors: Gael Varoquaux
# License: BSD 3 clause (C) INRIA 2014

from time import time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not introduce perf_counter that is never used in the documentation and stick to time.

clustering.fit(X_red)
print("%s :\t%.2fs" % (linkage, time() - t0))
print("%s :\t%.2fs" % (linkage, perf_counter() - t0))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, here we can use an f-string and the same in the title in plot_clustering below.

str(y[i]),
color=plt.cm.nipy_spectral(labels[i] / 10.0),
fontdict={"weight": "bold", "size": 9},
for t in np.sort(np.unique(y)):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of make the sorting we can load earlier the full Bunch:

digits = datasets.load_digits()
X, y = digits.data, digits.target

and then use the target_names that will be the array that you are expecting with the unique + sort.

Suggested change
for t in np.sort(np.unique(y)):
for digit in digits.target_names:

*X_red[y == t].T,
marker=f"${t}$",
s=50,
c=plt.cm.nipy_spectral(labels[y == t] / 10),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c=plt.cm.nipy_spectral(labels[y == t] / 10),
c=plt.cm.nipy_spectral(labels[y == digit] / 10),

@glemaitre glemaitre changed the title Accelerate_examples plot_digits_linkage.py MNT Accelerate_examples plot_digits_linkage.py Nov 22, 2021
@adrinjalali adrinjalali mentioned this pull request Nov 22, 2021
41 tasks
@adrinjalali
Copy link
Member

Closing since the other PR was opened before this one. Please check if there's an open PR before working on an example @siavrez

@adrinjalali
Copy link
Member

Sorry, didn't see that we had already merged that one.

@adrinjalali adrinjalali reopened this Nov 22, 2021
Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @siavrez

An illustration of various linkage option for agglomerative clustering on
a 2D embedding of the digits dataset.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only to revert these small changes here and we can merge this one.

@glemaitre glemaitre merged commit 84c655c into scikit-learn:main Nov 26, 2021
@glemaitre
Copy link
Member

Thanks @siavrez

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 29, 2021
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants