Skip to content

ENH: add top_k#446

Open
ev-br wants to merge 1 commit into
data-apis:mainfrom
ev-br:top_k
Open

ENH: add top_k#446
ev-br wants to merge 1 commit into
data-apis:mainfrom
ev-br:top_k

Conversation

@ev-br

@ev-br ev-br commented Jul 1, 2026

Copy link
Copy Markdown
Member

Add basic top_k wrappers, per data-apis/array-api#722
-tests tracker: data-apis/array-api-tests#438

Supersedes and closes gh-158

To test locally: with this PR and data-apis/array-api-tests#438, use

$ ARRAY_API_TESTS_SKIP_DTYPES=uint16,uint32,uint64 ARRAY_API_TESTS_VERSION=draft ARRAY_API_TESTS_MODULE=array_api_compat.torch pytest array_api_tests/test_searching_functions.py::test_top_k -s --max-examples=5000

Copilot AI review requested due to automatic review settings July 1, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds an Array API-compatible top_k wrapper to the array_api_compat.torch namespace, mapping the standardized signature to torch.topk.

Changes:

  • Introduces top_k(a, k, *, axis, mode, **kwargs) wrapper in the Torch backend.
  • Validates mode and forwards arguments to torch.topk.
  • Exports top_k via the Torch backend __all__.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/array_api_compat/torch/_aliases.py
@ev-br

ev-br commented Jul 2, 2026

Copy link
Copy Markdown
Member Author

So torch.topk needs only a small tweak to make it spec-compliant, which is exactly the remit of array-api-compat.
numpy is getting a native implementation in 2.6 (?).

Next is what to do about older numpies and cupy, which do not have native implementations.
We technically could reuse the full argsort-based implementation, cf data-apis/array-api-strict#215, but it has a wrong big-O complexity, and I'd be loathe sneak it in.
So unless there are very strong opinions to the contrary, I'd leave them out of array-api-compat. Thoughts?

@rgommers

rgommers commented Jul 2, 2026

Copy link
Copy Markdown
Member

Older numpy/cupy: I can see the case either way. Pinging @betatim for scikit-learn needs, and @MaanasArora in case he has good ideas after working on the numpy implementation.

@betatim

betatim commented Jul 3, 2026

Copy link
Copy Markdown
Member

What are the options?

My two thoughts/inputs:

  • for scikit-learn we say that our array API support requires you to use the latest versions of all the dependencies. Older versions are "untested, might work or not". (I need to think about how this will work if we enable it the support by default)
  • having an implementation for every library in array-api-compat is what I'd expect. Even if that implementation is slower/less optimal than it could be. array-api-compat is a "functionality provider of last resort" - ideally top_k is in the actual library but if not it is better to have it here than not at all. The alternative is that we write our own implementation in scikit-learn which would suffer from the same problems as the one here but is more work for scikit-learn and benefits less people

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.

4 participants