Skip to content

Conversation

tataganesh
Copy link
Contributor

Reference Issues/PRs

See also #27508

What does this implement/fix? Explain your changes.

DBSCAN modifies the precomputed sparse matrix passed to the fit method, which can lead to undesirable behaviour. This PR creates a copy of the sparse matrix before it is modified.

@github-actions
Copy link

github-actions bot commented Oct 24, 2023

✔️ Linting Passed

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

Generated for commit: 595399e. Link to the linter CI: here

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.

We will need an entry in the changelog and mention it as a fix as well.

@glemaitre glemaitre changed the title FIX: Copy Sparse Matrix to avoid in-place modification in DBSCAN FIX don't modify X in-place when precomputed in DBSCAN Oct 30, 2023
@tataganesh tataganesh changed the title FIX don't modify X in-place when precomputed in DBSCAN FIX: Don't modify X in-place when precomputed in DBSCAN Oct 30, 2023
@tataganesh tataganesh changed the title FIX: Don't modify X in-place when precomputed in DBSCAN FIX don't modify X in-place when precomputed in DBSCAN Oct 30, 2023
@glemaitre glemaitre self-requested a review November 14, 2023 13:23
# the sparse matrix. If `X` is modified in-place, the zeros from
# the diagonal will be made explicit.
np.fill_diagonal(X, 0)
X = csr_container(X)
Copy link
Member

Choose a reason for hiding this comment

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

We could make an assert just below this line to be sure that we have implicit zeros.

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.
Thanks @tataganesh

@tataganesh tataganesh requested a review from glemaitre November 16, 2023 07:03
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.

Still +1 on my side.

We will need an additional reviewer.

@glemaitre glemaitre added the Waiting for Second Reviewer First reviewer is done, need a second one! label Nov 16, 2023
Copy link
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @tataganesh

@OmarManzoor
Copy link
Contributor

@tataganesh Could you kindly resolve the conflict?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:cluster Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants