-
-
Notifications
You must be signed in to change notification settings - Fork 26.5k
ENH Added dtype preservation to Birch
#22968
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
Conversation
thomasjpfan
left a comment
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.
Thank you for the PR!
|
@Micky774 can you do a quick benchmark to compare your PR and main on float32 to be sure it does not introduce any performance regression ? |
@jeremiedbb Benching w/ # %%
from sklearn.cluster import Birch
from sklearn.datasets import make_blobs
X, y = make_blobs(n_samples=1000)
brc = Birch(n_clusters=None)
# %%
%timeit brc.fit_transform(X)branch: No (visible) performance regression. |
thomasjpfan
left a comment
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.
Please add an changelog item in doc/whats_new/v1.1.rst
thomasjpfan
left a comment
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.
LGTM
- Replaced `_CFNode.dtype` with `_CFNode.init_centroids.dtype`
jeremiedbb
left a comment
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.
Thanks @Micky774. Please add 2 tests: a test that checks that the results of transform are close (rtol=1e-4) between float32 and float64 input, and one that checks that the subcluster_centers_ attribute has the same type as X (parametrized over float32 and float64).
|
@jeremiedbb Wanted to check if you had any other feedback on this PR -- thanks :) |
jeremiedbb
left a comment
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 minor comments, otherwise LGTM.
Co-authored-by: Jérémie du Boisberranger <[email protected]>
|
Thanks @Micky774 ! |
Reference Issues/PRs
Addresses #11000
What does this implement/fix? Explain your changes.
Added
dtypepreservation toBirchAny other comments?