Skip to content

Conversation

yuejiaointel
Copy link
Contributor

@yuejiaointel yuejiaointel commented Sep 30, 2025

Description

Follow up PR of #2284 (will rebase after this one is merged)
that refactor neighbors with array api standard


Checklist:

Completeness and readability

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least a summary table with measured data, if performance change is expected.
  • I have provided justification why performance and/or quality metrics have changed or why changes are not expected.
  • I have extended the benchmarking suite and provided a corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@icfaust icfaust mentioned this pull request Sep 30, 2025
13 tasks

from onedal.utils.validation import _check_array

X = _check_array(X, accept_sparse="csr", dtype=[np.float64, np.float32])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it extract the namespace to get the dtypes, as done elsewhere? For example:

dtype=[xp.float64, xp.float32],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are lots of parts in onedal neighbor we need to move to sklearnex, and I plan to do array api stuff after this step to avoid too much changes at once

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have all the changes in one PR as they'd be modifying the same parts.

if X is not None:
check_feature_names(self, X, reset=False)
# Perform preprocessing at sklearnex level
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

NumPy is a mandatory dependency so it could be imported at the top level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, removed!

@yuejiaointel yuejiaointel force-pushed the refactor_neighbor_array_api branch from 25b3930 to debfcdf Compare October 7, 2025 18:07
@yuejiaointel yuejiaointel force-pushed the refactor_neighbor_array_api branch from a569e0c to 62c8ddd Compare October 9, 2025 05:33
@david-cortes-intel
Copy link
Contributor

As part of the PR, please add the relevant classes that will get array api support to this list now that they are documented:

Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 84.49438% with 69 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
sklearnex/neighbors/common.py 80.00% 25 Missing and 10 partials ⚠️
onedal/neighbors/neighbors.py 42.42% 16 Missing and 3 partials ⚠️
sklearnex/neighbors/knn_regression.py 88.09% 8 Missing and 2 partials ⚠️
sklearnex/neighbors/knn_unsupervised.py 90.47% 2 Missing and 2 partials ⚠️
sklearnex/neighbors/knn_classification.py 98.68% 0 Missing and 1 partial ⚠️
Flag Coverage Δ
azure ?
github 81.52% <84.49%> (+8.50%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
sklearnex/neighbors/_lof.py 100.00% <100.00%> (ø)
sklearnex/neighbors/knn_classification.py 98.54% <98.68%> (-0.06%) ⬇️
sklearnex/neighbors/knn_unsupervised.py 94.73% <90.47%> (-3.51%) ⬇️
sklearnex/neighbors/knn_regression.py 92.70% <88.09%> (-5.66%) ⬇️
onedal/neighbors/neighbors.py 61.64% <42.42%> (-18.41%) ⬇️
sklearnex/neighbors/common.py 86.12% <80.00%> (-6.36%) ⬇️

... and 75 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants