Skip to content

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

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.

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

Merged
merged 11 commits into from
Sep 25, 2023

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Aug 31, 2023

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

github-actions bot commented Aug 31, 2023

✔️ Linting Passed

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

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

@glemaitre glemaitre self-requested a review September 12, 2023 09:59
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.

@Charlie-XIAO
Copy link
Contributor Author

@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
Member

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
Contributor Author

@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

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.

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

@glemaitre
Copy link
Member

Maybe @OmarManzoor you want to have a look.

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.

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
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants