8000 [MRG] fix incorrect clusters due to dtype mismatch by amy12xx · Pull Request #17995 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] fix incorrect clusters due to dtype mismatch #17995

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 4 commits into from
Jul 26, 2020
Merged

[MRG] fix incorrect clusters due to dtype mismatch #17995

merged 4 commits into from
Jul 26, 2020

Conversation

amy12xx
Copy link
Contributor
@amy12xx amy12xx commented Jul 26, 2020

Reference Issues/PRs

Fixes #10832

What does this implement/fix? Explain your changes.

Minimal fix for the issue: Incorrect Clusters Due To Dtype Mismatch, by preserving the array dtype while calculating the S matrix.

Any other comments?

@amy12xx amy12xx changed the title [DRAFT] fix incorrect clusters due to dtype mismatch [MRG] fix incorrect clusters due to dtype mismatch Jul 26, 2020
rth
rth approved these changes Jul 26, 2020
View reviewed changes
Copy link
Member
@rth rth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @amy12xx ! Minor comment otherwise LGTM.

ogrisel
ogrisel approved these changes Jul 26, 2020
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM as well. As discussed on the EuroPython sprint channel, this is the minimal fix to get the bug fixed. The original issue reporter suggested we could ensure that all temporary arrays used in this method could be converted to follow the dtype precision of the input data but that would be a lot more invasive so we decided just to focus on fixing the bug while keeping the rest of the computation in float64.

@amy12xx @rth
Update doc/whats_new/v0.24.rst
ee38eda 
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
@rth rth merged commit 3cb3d41 into scikit-learn:master Jul 26, 2020
@ogrisel ogrisel added this to the 0.23.2 milestone Aug 3, 2020
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 3, 2020
@amy12xx @glemaitre
FIX AffinityPropagation fix results with float32 input (scikit-learn#…
07faae3 
…17995)
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@amy12xx @jayzed82
FIX AffinityPropagation fix results with float32 input (scikit-learn#…
5269a59 
…17995)
6FF2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@rth rth rth approved these changes

Assignees
No one assigned
Labels
module:cluster
Projects
None yet
Milestone
0.23.2
Development

Successfully merging this pull request may close these issues.

Incorrect Clusters Due To Dtype Mismatch
3 participants
@amy12xx @ogrisel @rth
0