Skip to content

TST Extend tests for scipy.sparse/*array in sklearn/neighbors/tests/test_neighbors#27250

Merged
OmarManzoor merged 11 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_neighbors
Sep 25, 2023
Merged

TST Extend tests for scipy.sparse/*array in sklearn/neighbors/tests/test_neighbors#27250
OmarManzoor merged 11 commits intoscikit-learn:mainfrom
Charlie-XIAO:tst_sp_neighbors

Conversation

@Charlie-XIAO
Copy link
Copy Markdown
Contributor

@Charlie-XIAO Charlie-XIAO commented Aug 31, 2023 •
edited
Loading

Towards #27090.

Made some (maybe undesired) changes to pass the tests. Maybe I shouldn't have done so? Please let me know if this is not the proper way to go.

@github-actions
Copy link
Copy Markdown

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

✔️ Linting Passed

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

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

@jjerphan jjerphan mentioned this pull request Aug 31, 2023
Closed
@glemaitre glemaitre self-requested a review September 12, 2023 09:59
glemaitre
glemaitre reviewed Sep 12, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@Charlie-XIAO
Copy link
Copy Markdown
Contributor Author

Charlie-XIAO commented Sep 12, 2023

@glemaitre There change seems to (possibly) affect these public functionality:

  • metrics.pairwise_distances when metric is a callable: X, Y can be sparse arrays
  • metrics.pairwise.pairwise_kernels when metric is a callable: X, Y can be sparse arrays
  • cluster.compute_optics_graph when metric is a callable: X can be sparse array
  • cluster.HDBSCAN.fit when metric is a callable and when one of the following holds: (1) algorithm is "brute" (2) algorithm is "auto" (and X is sparse) (3) store_centers is not None: X can be sparse array
  • manifold.trustworthiness when metric is a callable: X can be sparse array
  • manifold.TSNE.fit when metric is a callable and method is "exact": X can be sparse array
  • manifold.TSNE.fit_transform when metric is a callable and method is "exact": X can be sparse array
  • metrics.pairwise_distances_chuncked when metric is a callable: X, Y can be sparse arrays
  • manifold.Isomap.fit when metric is a callable: X can be sparse array
  • manifold.Isomap.fit_transform when metric is a callable: X can be sparse array
  • cluster.OPTICS.fit when metric is a callable: X can be sparse array

However I still need to test a bit so as to confirm that these functionality indeed changed.

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Sep 12, 2023

I see a list growing. I assume that we should use a common entry in the changelog stating the estimators supporting the sparse arrays and add all PRs number and contributors in this entry.
You can ignore it for the moment. I will try to that at some point in main.

@Charlie-XIAO
Copy link
Copy Markdown
Contributor Author

Charlie-XIAO commented Sep 22, 2023

@glemaitre Seems that you have created that common entry in on main. I have now updated the changelog there, and validated that the functions and classed mentioned are indeed affected. Would you please take a look?

On the main branch:

===============================================================================================================================
                                                       Testing functions
===============================================================================================================================

`cluster.compute_optics_graph` when `metric` is callable and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`manifold.trustworthiness` when `metric` is callable and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`metrics.pairwise_distances` when `metric` is callable and `X` and `Y` are sparrays    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`metrics.pairwise_distances_chunked` when `metric` is callable and `X` and `Y` are sparrays    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`metrics.pairwise.pairwise_kernels` when `metric` is callable and `X` and `Y` are sparrays    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`

===============================================================================================================================
                                                        Testing classes
===============================================================================================================================

`cluster.HDBSCAN` when `metric` is callable, `algorithm` is 'brute' or 'auto', and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`cluster.OPTICS` when `metric` is callable and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`manifold.Isomap` when `metric` is callable and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`
`manifold.TSNE` when `metric` is callable and `X` is sparray    NotImplementedError: We have not yet implemented 1D sparse slices; please index using explicit indices, e.g. `x[:, [0]]`

In this PR:

===============================================================================================================================
                                                       Testing functions
===============================================================================================================================

`cluster.compute_optics_graph` when `metric` is callable and `X` is sparray    Passed
`manifold.trustworthiness` when `metric` is callable and `X` is sparray    Passed
`metrics.pairwise_distances` when `metric` is callable and `X` and `Y` are sparrays    Passed
`metrics.pairwise_distances_chunked` when `metric` is callable and `X` and `Y` are sparrays    Passed
`metrics.pairwise.pairwise_kernels` when `metric` is callable and `X` and `Y` are sparrays    Passed

===============================================================================================================================
                                                        Testing classes
===============================================================================================================================

`cluster.HDBSCAN` when `metric` is callable, `algorithm` is 'brute' or 'auto', and `X` is sparray    Passed
`cluster.OPTICS` when `metric` is callable and `X` is sparray    Passed
`manifold.Isomap` when `metric` is callable and `X` is sparray    Passed
`manifold.TSNE` when `metric` is callable and `X` is sparray    Passed

glemaitre
glemaitre approved these changes Sep 25, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for updating the changelog.
It looks good to me.

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Sep 25, 2023

Maybe @OmarManzoor you want to have a look.

OmarManzoor
OmarManzoor approved these changes Sep 25, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Otherwise LGTM. Thanks @Charlie-XIAO

@OmarManzoor OmarManzoor enabled auto-merge (squash) September 25, 2023 10:36
@OmarManzoor OmarManzoor merged commit d38a7e3 into scikit-learn:main Sep 25, 2023
@Charlie-XIAO Charlie-XIAO deleted the tst_sp_neighbors branch September 25, 2023 12:16
lesteve pushed a commit to lesteve/scikit-learn that referenced this pull request Sep 28, 2023
@Charlie-XIAO @lesteve
TST Extend tests for scipy.sparse/*array in `sklearn/neighbors/test…
6e978b8 
…s/test_neighbors` (scikit-learn#27250)
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@Charlie-XIAO @REDVM
TST Extend tests for scipy.sparse/*array in `sklearn/neighbors/test…
0ead213 
…s/test_neighbors` (scikit-learn#27250)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees

No one assigned

Labels

module:metrics module:neighbors module:utils No Changelog Needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Charlie-XIAO @glemaitre @OmarManzoor @jjerphan