fix: keep element order when writing F-contiguous chunks with vlen codecs#4116
Conversation
github-actions
Bot
added
needs release notes
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4116 +/- ##
==========================================
+ Coverage 93.54% 93.62% +0.08%
==========================================
Files 90 90
Lines 11936 11936
==========================================
+ Hits 11165 11175 +10
+ Misses 771 761 -10
🚀 New features to boost your workflow:
|
|
thanks for this fix! Could you check if the other numcodecs codecs (delta, fixedscaleoffset, etc) are affected by the same bug? |
|
Checked, and yes: Delta, FixedScaleOffset and PackBits all round-trip transposed data for an F-contiguous chunk that exactly covers the selection, both when calling the numcodecs codec directly and through a zarr array with the filter configured. The path is the generic adapter in codecs/numcodecs/_codecs.py, which hands chunk_data.as_ndarray_like() to numcodecs unchanged while decode reshapes in C order, so every array-array filter goes through the same exposed path. I pushed the same ascontiguousarray treatment for the array-array and array-bytes adapter encode paths, with regression tests for the three filters above and the changelog entry updated to match. One probe gotcha worth mentioning for review: a test pattern that is symmetric under transpose passes even on broken code (arange % 3 on a 16-wide array is symmetric, since 16 ≡ 1 mod 3), so the tests use arange and a lower-triangular bool mask. I'll open the numcodecs issue about the underlying order='A' behavior next, since direct numcodecs users stay exposed either way. |
…decs The numcodecs vlen codecs flatten their input with order='A', so an F-contiguous object-dtype chunk is encoded in transposed element order while decode reshapes in C order, silently scrambling the round-trip. Make both vlen codec wrappers pass a C-contiguous array to numcodecs; this copies only the object-pointer array and only when the chunk is not already C-contiguous.
The generic numcodecs adapter codecs pass the chunk to numcodecs unchanged, so F-contiguous chunks hit the same order='A' flattening as the vlen codecs: Delta, FixedScaleOffset and PackBits all round-trip transposed data for an F-contiguous chunk that exactly covers the selection. Apply the same C-contiguity normalization in the array-array and array-bytes adapter encode paths.
oldrobotdev
force-pushed
the
fix-vlen-f-contiguous-write
branch
from
96b892e to
c0be9f4
Compare
d-v-b
left a comment
d-v-b
left a comment
There was a problem hiding this comment.
pending CI, this looks great! thanks for the fix.
Follow-up test-quality cleanup for the zarr-developersgh-3558 fix (zarr-developers#4116): - Merge test_vlen_string_f_contiguous and test_vlen_bytes_f_contiguous into one test parametrized over (dtype, fill_value, elements), matching the parametrized style of test_vlen_string in the same file. - Drop the chunks=(2,2) case: with (3,3) data it produces only partial chunks, which the codec pipeline recopies to C order before encoding, so it never exercised the F-contiguous path it claimed to cover. Use chunks == shape (a single complete chunk) so the F-contiguous buffer reaches the codec untouched. - Build the F-contiguous input directly via reshape(order="F") instead of wrapping a C-order reshape in np.asarray(..., order="F"), and assert data.flags.f_contiguous so a future simplification can't silently defeat the regression guard. - Add a docstring stating the verified behavior. Assisted-by: ClaudeCode:claude-opus-4.8
2 participants
Summary
Fixes #3558. Writing an F-contiguous object-dtype array to a chunked string or bytes array scrambles the stored data whenever a chunk is covered exactly by the selection: the write path passes the chunk view to numcodecs without a copy, numcodecs flattens it with order='A' (column-major for F-contiguous input), and decode reshapes in C order. The wrappers in codecs/vlen_utf8.py now pass np.ascontiguousarray(...) to the codec, a no-op for C-contiguous chunks and a copy of the object-pointer array otherwise. Regression tests cover vlen-utf8 and vlen-bytes at both the exact-cover and partial-chunk shapes; the exact-cover cases fail on current main.
For reviewers
The one design question is the fix layer: this could live in numcodecs instead (change the flatten to C order), but that touches the v2 out= decode path where the old behavior was self-consistent for F-ordered arrays, so I kept the change on the zarr side where decode already commits to C order. The repro and codec-level probe are in #3558.
Author attestation
TODO
docs/user-guide/*.md(not applicable)changes/