fix(python): handle non-row-major and zero-size input arrays - #28
Merged
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Two input-handling bugs in the bindings, both reachable from Zarr: 1. The bindings handed the input array's raw buffer to the conversion kernels via numpy::PyReadonlyArray::as_slice, whose guard accepts column-major arrays as well as row-major ones. The kernels then wrote those elements into a freshly allocated row-major output of the same shape, silently transposing the data. Arrays that were neither C- nor F-contiguous were rejected with "Input array must be contiguous". The Zarr transpose codec hands the next codec a transposed view -- column-major in 2-D, strided in higher dimensions -- so both cases occur in routine pipelines (zarr-developers/zarr-python#4237). The Python wrapper now normalizes the input with np.asarray(arr, order="C"): a no-op for row-major arrays, a copy for anything else, and 0-d-preserving (unlike np.ascontiguousarray). The binding keeps a strict backstop for direct callers of the private module, rejecting non-row-major input instead of misreading it. Normalizing on the Rust side instead (ndarray's as_standard_layout) was tried and rejected: ndarray's raw-view stride assertions panic on layouts numpy considers legal, and numpy's own normalization is authoritative. 2. numpy gives every zero-size array strides of 0, which ndarray's debug-build stride assertions reject as self-overlapping -- so casting something as plain as np.zeros((4, 0)) panicked in any maturin develop build (release builds compile the assertion out and were unaffected). Zero-size arrays now skip the conversion entirely; there is nothing to convert. Also corrects the cast_array_into output error message, which said "contiguous" where it meant row-major, and gives cast_array_into a Python wrapper (it was previously re-exported raw) so both entry points normalize identically. Assisted-by: ClaudeCode:claude-opus-4.8
Example-based coverage: parametrize the memory-layout tests over one dtype pair per conversion path (float->int, int->int, float->float, int->float) plus a float16 source, for both cast_array and cast_array_into, with shared fixtures in conftest. Dedicated cases pin the SIMD clamp fast path and scalar-map matching on non-contiguous views, and a backstop test pins the private module's rejection of non-row-major input. Property-based coverage (hypothesis): layout invariance -- casting an arbitrarily-strided view must behave exactly like casting its C-contiguous copy, and cast_array_into must agree with cast_array -- sampled over the full dtype grid and random transpose/slice views. These properties found the zero-size-array panic that the hand-picked empty-array case missed. test_non_contiguous_input is removed: it asserted the rejection that was itself the bug. Assisted-by: ClaudeCode:claude-opus-4.8
d-v-b
force-pushed
the
fix/non-row-major-input
branch
from
August 12, 2026 16:16
fdadd47 to
8d161f9
Compare
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Fixes the silent data corruption behind zarr-python#4237, plus a debug-build panic on zero-size arrays that property-based testing found along the way.
Bug 1: non-row-major input is misread or rejected
The bindings handed the input array's raw buffer to the conversion kernels via
numpy::PyReadonlyArray::as_slice. That guard accepts column-major arrays as well as row-major ones:The kernels then wrote those elements into a freshly allocated row-major output of the same shape, silently transposing the data:
No exception, no warning. Arrays that were neither C- nor F-contiguous took the other branch and were rejected with
Input array must be contiguous. Both cases occur in routine Zarr pipelines: thetransposecodec hands the next codec a transposed view — column-major in 2-D, strided in higher dimensions. TheValueErrorreported in zarr-python#4237 was the lucky branch; 2-D arrays corrupted quietly.Fix: the Python wrapper normalizes the input with
np.asarray(arr, order="C")— a no-op for row-major arrays, a copy for anything else, and 0-d-preserving (unlikenp.ascontiguousarray). The binding keeps a strict backstop for direct callers of the private extension module, rejecting non-row-major input instead of misreading it.cast_array_intogains a Python wrapper (it was previously re-exported raw) so both entry points normalize identically.An earlier revision of this PR normalized on the Rust side with
ndarray'sas_standard_layout. The property tests falsified it:ndarray's raw-view stride assertions panic on layouts numpy considers legal (e.g. a zero-size slice with negative strides), so numpy's own normalization is the authoritative place to do this.Bug 2: zero-size arrays panic in debug builds (pre-existing on
main)numpy gives every zero-size array strides of 0, and
ndarray's debug-build stride assertions reject a 0-stride dim of size > 1 as self-overlapping. So this panics on anymaturin developbuild ofmain:Release builds compile the assertion out, which is why published wheels seemed unaffected. Zero-size arrays now skip the conversion block entirely — there is nothing to convert, and the
ndarrayview over the output buffer is never constructed.Also corrects the
cast_array_intooutput error message, which said "contiguous" where it meant row-major.Tests
Example-based: the memory-layout tests are parametrized over 8 layouts (row-major, column-major, 3-D transpose, strided, negative-stride, sliced view, 0-d, empty) × one dtype pair per conversion path (float→int, int→int, float→float, int→float) plus a float16 source, for both
cast_arrayandcast_array_into— 80 cases. Dedicated cases pin the SIMD clamp fast path and scalar-map matching on non-contiguous views, and a backstop test pins the private module's strict rejection.test_non_contiguous_inputis removed: it asserted the rejection that was itself the bug.Property-based (new
hypothesistest dependency): layout invariance — casting an arbitrarily-strided view must behave exactly like casting its C-contiguous copy (same values, same shape, or the same error), andcast_array_intomust agree withcast_array— sampled over the full 11×11 dtype grid and random transpose/slice views. These properties falsified the earlier Rust-side revision of this fix and found bug 2, which the hand-picked empty-array case missed ((0, 3)happens to slip past the assertion;(4, 0)does not).Kill-test results: with the source fix reverted, 56 of 152 tests fail — corruption caught by
AssertionError, not justValueError, across every conversion path. With only the wrapper fix applied against the old Rust code, exactly 4 fail, one per Rust-side change, so no part of the fix is untested.Verification
pytest— 152 passed; property stress run at 2000 fresh-seed examples cleancargo test -p zarr-cast-value— 38 passed;cargo fmt --checkandcargo clippycleanmainwith no zarr-side patch — all transpose/order/filter-order combinations round-trip correctly, 0-d arrays work,tests/test_codecs/774 passedDownstream
zarr-python#4238 works around bug 1 with
np.ascontiguousarrayon the zarr side. Its regression test passes against unpatched zarr-python once this ships, so the workaround can be dropped — and should be either way, sincenp.ascontiguousarraypromotes 0-d arrays to shape(1,)and breaks 0-d Zarr arrays. zarr-python will want a minimumcast-value-rsbump once this is released.Sibling repo
cast-value.pyneeds no change: its numpy implementation is stride-safe, andCastValueRustV1forwards tocast_array, so it inherits this fix.