Skip to content

FIX Avoid modifying X in-place when precomputed in OPTICS #28491

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 14 commits into from
Feb 26, 2024
Merged

FIX Avoid modifying X in-place when precomputed in OPTICS #28491

merged 14 commits into from
Feb 26, 2024

Conversation

lamdang2k
Copy link
Contributor

Reference Issues/PRs

See also #27508

What does this implement/fix? Explain your changes.

Create a copy of the sparse matrix X when metric=precomputed to prevent OPTICS from modify the input in-place

Any other comments?

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 20, 2024 •
edited
Loading

✔️ Linting Passed

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

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

Charlie-XIAO
Charlie-XIAO reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO 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.

LGTM according to #27651, except for some nitpicks in changelog as follows. I reran the failing CI because it is not likely caused by the changes in this PR. Thanks @lamdang2k!

@lamdang2k
Copy link
Contributor Author

LGTM according to #27651, except for some nitpicks in changelog as follows. I reran the failing CI because it is not likely caused by the changes in this PR. Thanks @lamdang2k!

Thank you for your prompt review. I have made the changes as you suggested

Charlie-XIAO
Charlie-XIAO approved these changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO left a comment

Choose a reason for hiding this comment

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

I resolved the merge conflicts and moved sklearn.cluster to above sklearn.compose which should be the correct alphabetical order. Thanks for the PR!

@lamdang2k
Copy link
Contributor Author

I resolved the merge conflicts and moved sklearn.cluster to above sklearn.compose which should be the correct alphabetical order. Thanks for the PR!

After your merge there are some failed tests. I'll look on them

@Charlie-XIAO
Copy link
Contributor

Charlie-XIAO commented Feb 25, 2024 •
edited
Loading

Those are likely caused by scipy/scipy#20060 and would be solved with #28524. You may just leave them as is for now.

ogrisel
ogrisel approved these changes Feb 26, 2024
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.

@ogrisel
Copy link
Member

ogrisel commented Feb 26, 2024

Let's wait for #28524 to be merged to main first and then sync this PR to main and re-run the CI on this PR and auto-merge if green.

@ogrisel ogrisel enabled auto-merge (squash) February 26, 2024 09:22
@ogrisel ogrisel merged commit 845be98 into scikit-learn:main Feb 26, 2024
@lamdang2k lamdang2k deleted the fix-copy-x branch March 7, 2024 22:29
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

@Charlie-XIAO Charlie-XIAO Charlie-XIAO approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@lamdang2k @Charlie-XIAO @ogrisel @glemaitre