Skip to content

ENH Add sparse input support to OPTICS#22965

Merged
jeremiedbb merged 41 commits intoscikit-learn:mainfrom
Micky774:sparse_optics
May 2, 2022
Merged

ENH Add sparse input support to OPTICS#22965
jeremiedbb merged 41 commits intoscikit-learn:mainfrom
Micky774:sparse_optics

Conversation

@Micky774
Copy link
Copy Markdown
Contributor

@Micky774 Micky774 commented Mar 27, 2022 •
edited
Loading

Reference Issues/PRs

Fixes #11982
Resolves #14736 (stalled)
Resolves #20802 (stalled)

What does this implement/fix? Explain your changes.

PRs #14736 and #20802: Add support for sparse matrices in OPTICS.fit
This PR: updates work and provides continuation for review.

Any other comments?

huntzhan and others added 21 commits August 23, 2019 11:53
@huntzhan
Try csr support.
5f7e6de
@huntzhan
Change the default metric of OPTICS to euclidean.
18425aa
@huntzhan
Retain the default metric minkowski.
f712b46
@huntzhan
Undo tests.
876bb29
@huntzhan
Fix flake8.
71b5c1e
@huntzhan
Add sparse tests.
6f498a9
@elefantinerrrrs
Merged huntzhan
7cddb87
@elefantinerrrrs
Change assert_raise_message to pytest.raises
0f832d4
@elefantinerrrrs
Parametrized tests
b237b05
@elefantinerrrrs
Fix flake8 test_optics.py
a003010
@elefantinerrrrs
Fix flake8 _optics.py
2188e5b
@elefantinerrrrs
Add sparse precomputed test case
f8a43e4
@elefantinerrrrs
Black test_optics.py
52fdf93
@elefantinerrrrs
Merge remote-tracking branch 'upstream/main' into add-optics-sparse
9dceabd
@elefantinerrrrs
Black _optics.py
8992c87
@elefantinerrrrs
Add sparse matrix support to _optics.py
91fd5ed 
Original commit was pushed, but wasn't reflected on github for some reason
@elefantinerrrrs
Merge remote-tracking branch 'origin/add-optics-sparse' into add-opti…
80c2a73 
…cs-sparse
@elefantinerrrrs
Added changelog entry
53474d2
@elefantinerrrrs
Merge branch 'main' into add-optics-sparse
611cf77
@Micky774
Merge branch 'main' into sparse_optics
2d4a4d3
@Micky774
Removed extra changelog entry
f15c019
@Micky774 Micky774 changed the title [MRG] Make OPTICS support sparse input [MRG] Add sparse input support to OPTICS Mar 27, 2022
jeremiedbb
jeremiedbb reviewed Mar 28, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Not sure what to think about these warnings. Maybe we should not filter them since they indicate that the use of NearestNeighbors is not optimal. At least maybe there's something we can do to the precomputed dist matrix to not trigger all of them.

I think not all tests need to be parametrized by metric and run on dense and sparse, especially those who just do error checking or sanity checks.

@Micky774
Copy link
Copy Markdown
Contributor Author

Micky774 commented Apr 1, 2022

@jeremiedbb I have

  1. Converted lil_matrix constructions to csr_matrix
  2. Improved the docstring for metric by including a line stating that Sparse matrices are only supported by scikit-learn metrics.
  3. Removed the warning filters so that they are passed regularly
  4. Streamlined the tests by removing sparse parametrization for several tests, keeping the parametrization only for testing the correctness of the algorithm, DBSCAN parity, and the ability to input sparse precomputed distances.

@Micky774
Copy link
Copy Markdown
Contributor Author

Micky774 commented Apr 3, 2022

Note that the failing test is test_ridge_regression_vstacked_X[wide-5-True-sag] and consequently not related to this PR

jeremiedbb
jeremiedbb reviewed Apr 4, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Looks good. I just think that you added some unreachable code, but that needs confirmation

jeremiedbb
jeremiedbb reviewed Apr 7, 2022
View reviewed changes
jeremiedbb
jeremiedbb approved these changes Apr 29, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM besides the sparse efficiency warning thing.

@jeremiedbb jeremiedbb added this to the 1.2 milestone Apr 29, 2022
thomasjpfan
thomasjpfan reviewed Apr 30, 2022
View reviewed changes
Micky774 and others added 2 commits April 30, 2022 18:14
@Micky774
Merge branch 'main' into sparse_optics
2ae23a9
@Micky774 @thomasjpfan
Update sklearn/cluster/_optics.py
27f3edd 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@thomasjpfan thomasjpfan changed the title [MRG] Add sparse input support to OPTICS ENH Add sparse input support to OPTICS May 1, 2022
thomasjpfan
thomasjpfan approved these changes May 1, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

sklearn/cluster/_optics.py
Comment on lines +616 to +617
dists.sort_indices()
dists = dists.data
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan May 1, 2022

Choose a reason for hiding this comment

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

@jeremiedbb This is a change from your previous approval. Are you okay with this?

@jeremiedbb jeremiedbb merged commit af5b6a1 into scikit-learn:main May 2, 2022
@jeremiedbb
Copy link
Copy Markdown
Member

jeremiedbb commented May 2, 2022

Thanks @Micky774 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees

No one assigned

Labels

module:cluster

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

OPTICS should support sparse matrices and precomputed distances

5 participants

@Micky774 @jeremiedbb @thomasjpfan @huntzhan @elefantinerrrrs