-
-
Notifications
You must be signed in to change notification settings - Fork 26.1k
Add array API support for Nystroem approximation #29661
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
base: main
Are you sure you want to change the base?
Conversation
While working on this I kept running into this error @OmarManzoor do you happen to have any pointer on how to get around this when you worked on #29433 ? Thanks!
|
There is something fishy that happens right after sklearn/metrics/tests/test_common.py:1979: in check_array_api_metric_pairwise
check_array_api_metric(
X_np = array([[0.1, 0.2, 0.3],
[0.4, 0.5, 0.6]], dtype=float32)
Y_np = array([[0.2, 0.3, 0.4],
[0.5, 0.6, 0.7]], dtype=float32)
array_namespace = 'torch'
device = 'cpu'
dtype_name = 'float32'
metric = <function rbf_kernel at 0x125b99b20>
metric_kwargs = {}
sklearn/metrics/tests/test_common.py:1773: in check_array_api_metric
metric_xp = metric(a_xp, b_xp, **metric_kwargs)
a_np = array([[0.1, 0.2, 0.3],
[0.4, 0.5, 0.6]], dtype=float32)
a_xp = tensor([0.1000, 0.2000, 0.3000, 0.4000, 0.5000, 0.6000])
array_namespace = 'torch'
b_np = array([[0.2, 0.3, 0.4],
[0.5, 0.6, 0.7]], dtype=float32)
b_xp = tensor([0.2000, 0.3000, 0.4000, 0.5000, 0.6000, 0.7000])
|
Hi @EmilyXinyi I checked out your branch locally and ran the Also @ogrisel I think it might be useful to merge the PR #29475 as the current PR makes use of the other kernels. What do you think? |
Let me update the branch in this PR to check if the errors in the CI can be reproduced or if they were a transient event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback. In general, I think you should have a look at a merge PR that adds array API support to another unsupervised transformer (e.g. PCA):
This should give guidance on how to upgrade this estimator and test it.
@EmilyXinyi here are the instructions to update the changelog for this PR: https://github.com/scikit-learn/scikit-learn/tree/main/doc/whats_new/upcoming_changes#readme |
BTW @EmilyXinyi, once you have managed to fix the changelog problem of this PR, feel free to open another PR to improve the message of the failed changelog check to make it more informative. This is done in the |
What does this implement/fix? Explain your changes.
Make Nystroem approximation array API compatible
To Do