Skip to content

Keep the last value when setitem gets repeated indices - #4601

Open
bhaskargurram-ai wants to merge 3 commits into
zarr-developers:mainfrom
bhaskargurram-ai:fix/setitem-repeated-indices
Open

bhaskargurram-ai wants to merge 3 commits into
zarr-developers:mainfrom
bhaskargurram-ai:fix/setitem-repeated-indices

Conversation

@bhaskargurram-ai

Copy link
Copy Markdown
Contributor

When an index array repeats a position, oindex, vindex and __setitem__ could store any of the values given for it. The issue's example, arr.oindex[np.array([-1, -1, 0, 0])] = [0, 1, 2, 3] on a chunked array, gives [2, 0, 0, 0] on Linux with numpy 2 and [3, 0, 0, 1] on macOS, while NumPy itself gives [3, 0, 0, 1] everywhere. The reason is that the in-chunk scatter receives the repeated positions in whatever order the (unstable) argsort by chunk put them, so the "last" write is platform dependent, and for a single-chunk array the order is different again.

This follows the suggestion in the thread: before any chunk is touched, the selection is reduced so that it names each element once, keeping the last occurrence, and the value array is reduced to match. The stored value is then the last one given for the element, which is what sequential NumPy assignment produces, on every platform and for every chunking. For orthogonal selections the reduction is per dimension (that matches a[np.ix_(i, j)] = v, where the last row wins for a repeated i and the last column for a repeated j); for coordinate selections the points are reduced on their linear index. Negative and positive spellings of the same index count as the same position. Selections with no repeats take the existing path unchanged: the check is a linear pass for sorted index arrays and one np.unique or np.lexsort otherwise. Masks never repeat a point and are skipped.

A value that cannot be broadcast to the selection now raises ValueError: shape mismatch ... (NumPy's wording) on this path instead of silently writing a prefix of it.

Tests: the previously skipped test_setitem_repeated_index runs again, parametrised over chunkings and over oindex, vindex and __setitem__; new tests cover repeats along both dimensions of an orthogonal selection (with integer, slice and Boolean selectors on the other dimension, and scalar values), repeated points across chunk boundaries with 1-D and 2-D coordinate arrays and the sorted single-array path, the shape-mismatch error, and the last_occurrences helper. The hypothesis tests test_oindex and test_vindex no longer assume away repeated indices, so they now compare repeated-index writes against NumPy as well; before the change test_oindex fails within a few examples.

Fixes #2855

An orthogonal or coordinate selection that names the same element more
than once used to store whichever value the chunk dispatch and the
in-chunk scatter happened to write last, which depends on the chunk
layout and on the sort order of the platform's NumPy build. The
selection is now reduced, before any chunk is touched, to the last
occurrence of each element, with the value array reduced to match, so
the stored value is the last one given, as with NumPy, on every
platform and chunking. The property tests draw repeated indices again
instead of skipping them.
@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 94.86%. Comparing base (9b82146) to head (b35bc0c).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/zarr/core/indexing.py 98.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4601      +/-   ##
==========================================
+ Coverage   94.84%   94.86%   +0.02%     
==========================================
  Files          96       96              
  Lines       13997    14072      +75     
==========================================
+ Hits        13275    13349      +74     
- Misses        722      723       +1     
Files with missing lines Coverage Δ
src/zarr/core/array.py 98.47% <100.00%> (+0.02%) ⬆️
src/zarr/core/indexing.py 96.66% <98.11%> (+0.08%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@d-v-b d-v-b added the benchmark Code will be benchmarked in a CI job. label Nov 6, 2026
@codspeed

codspeed Bot commented Nov 6, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 113 untouched benchmarks
⏩ 37 skipped benchmarks1


Comparing bhaskargurram-ai:fix/setitem-repeated-indices (eb6d355) with main (9e44021)2

Open in CodSpeed

Footnotes

  1. 37 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (17c049c) during the generation of this report, so 9e44021 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

This branch has not been deployed

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

Labels

benchmark Code will be benchmarked in a CI job.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setitem with repeated indices is not guaranteed in practice

2 participants