Skip to content

fix: make topk() return numerically-keyed tuple for consistency - #30

Merged
CodeWithKyrian merged 1 commit into
mainfrom
fix/topk-return-consistency
Jun 9, 2026
Merged

CodeWithKyrian merged 1 commit into
mainfrom
fix/topk-return-consistency

Conversation

@CodeWithKyrian

Copy link
Copy Markdown
Contributor

This PR changes topk() from returning string-keyed arrays (['values' => ..., 'indices' => ...]) to numerically-keyed arrays ([$values, $indices]) to match the convention used by every other multi-NDArray return in the library.

Motivation and Context

topk() was the only method returning a string-keyed tuple (['values' => NDArray, 'indices' => NDArray]). All other multi-NDArray returns — svd(), qr(), eig(), eigh(), lstsq(), split(), vsplit(), hsplit(), meshgrid() — use numerical positional keys. This inconsistency meant callers had to remember different destructuring patterns depending on the method. The new signature matches NumPy and PyTorch semantics where destructuring is natural: [$values, $indices] = $arr->topk(3).

What's Changed

  • topk() and its private helpers (topkAxisOp, topkFlatOp) now return [NDArray, NDArray] instead of ['values' => NDArray, 'indices' => NDArray]
  • Updated PHPDoc @return annotations from array{values: NDArray, indices: NDArray} to array{0: NDArray, 1: NDArray}
  • Updated all tests to use positional destructuring ($topk[0]/$topk[1] instead of $topk['values']/$topk['indices'])
  • Updated API documentation examples

Breaking Changes

Yes. Anyone relying on $result['values'] or $result['indices'] to access topk results will need to update to $result[0]/$result[1] or destructure with [$values, $indices] = $arr->topk(...).

@CodeWithKyrian
CodeWithKyrian merged commit 513722b into main Jun 9, 2026
14 checks passed
@CodeWithKyrian
CodeWithKyrian deleted the fix/topk-return-consistency branch June 9, 2026 12:52
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.

1 participant