Skip to content

FIX Bisecting kmeans when data has 1 feature #27243

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 1, 2023
Merged

FIX Bisecting kmeans when data has 1 feature #27243

merged 3 commits into from
Sep 1, 2023

Conversation

jeremiedbb
Copy link
Member

Fixes #27236

@jeremiedbb jeremiedbb added the Quick Review For PRs that are quick to review label Aug 30, 2023
@jeremiedbb jeremiedbb added this to the 1.3.1 milestone Aug 30, 2023
@github-actions
Copy link

github-actions bot commented Aug 30, 2023 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: c868db0. Link to the linter CI: here

@jeremiedbb jeremiedbb mentioned this pull request Aug 30, 2023
Closed
betatim
betatim reviewed Aug 30, 2023
View reviewed changes
betatim
betatim reviewed Aug 30, 2023
View reviewed changes
betatim
betatim approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. The thing I don't understand is why this only happens if n_features=1. Wouldn't it happen always when n_features != 2?

@jeremiedbb
Copy link
Member Author

When n_features > 2 we allocate an array to big but since we only access the first 2 elements later there's no out of bound issue

@jeremiedbb @betatim
Update doc/whats_new/v1.3.rst
c868db0 
Co-authored-by: Tim Head <betatim@gmail.com>
@betatim
Copy link
Member

betatim commented Aug 30, 2023

Ah yes. So there is a bug for n_features > 2, but we don't "observe" it.

@jeremiedbb
Copy link
Member Author

Ah yes. So there is a bug for n_features > 2, but we don't "observe" it.

I wouldn't call it a bug but a sub-optimal memory management 😄

jjerphan
jjerphan approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM.

Is predict also affected?

@jeremiedbb
Copy link
Member Author

Is predict also affected?

Nope

@jjerphan jjerphan changed the title Fix bisecting kmeans when data has 1 feature FIX Bisecting kmeans when data has 1 feature Sep 1, 2023
@jjerphan jjerphan merged commit 8b7756e into scikit-learn:main Sep 1, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 5, 2023
@jeremiedbb @betatim @glemaitre
FIX Bisecting kmeans when data has 1 feature (scikit-learn#27243)
1e6f1c1 
Co-authored-by: Tim Head <betatim@gmail.com>
@thomasjpfan thomasjpfan mentioned this pull request Sep 5, 2023
Merged
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@jeremiedbb @betatim @glemaitre
FIX Bisecting kmeans when data has 1 feature (scikit-learn#27243)
29b81dd 
Co-authored-by: Tim Head <betatim@gmail.com>
jeremiedbb added a commit that referenced this pull request Sep 20, 2023
@jeremiedbb @betatim
FIX Bisecting kmeans when data has 1 feature (#27243)
9fcfa5d 
Co-authored-by: Tim Head <betatim@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@jeremiedbb @betatim @REDVM
FIX Bisecting kmeans when data has 1 feature (scikit-learn#27243)
2c10b30 
Co-authored-by: Tim Head <betatim@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betatim betatim betatim approved these changes

@jjerphan jjerphan jjerphan approved these changes

Assignees
No one assigned
Labels
module:cluster Quick Review For PRs that are quick to review
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.

BisectingKmeans - intertia per cluster
3 participants
@jeremiedbb @betatim @jjerphan