Preserve FixedScaleOffset array dimensions

[shixi-li]

Closes #852.

Summary

• stop flattening non-scalar arrays in FixedScaleOffset, so its elementwise transform preserves the shape needed by downstream codecs
• keep the encoded byte stream and codec configuration unchanged; no new metadata flag is introduced
• fix ndarray_copy centrally so equally shaped arrays retain logical coordinates across C/F memory-order differences, including shaped memoryview destinations
• preserve the historical one-element result for 0-D inputs

Serialization boundary

Raw serialized bytes do not contain shape or memory-order metadata, so decode(serialized) remains one-dimensional. The tests make that boundary explicit. When the caller supplies the correctly shaped output buffer, serialized C- and F-ordered data round-trip correctly.

The tests also compare the new encoded bytes with the legacy flattened implementation for both C and F inputs; they are byte-identical.

Validation

• pytest tests/test_fixedscaleoffset.py tests/test_compat.py -q — 24 passed
• pytest -q — 656 passed, 32 optional-dependency skips
• ruff check on all four changed source/test files — passed
• ruff format --check on all four changed source/test files — passed
• git diff --check — passed

Checklist

• Unit tests cover shaped encode/decode, serialized bytes, shaped output buffers, cross-order ndarray/memoryview copies, scalar compatibility, config compatibility, and byte compatibility
• Full locally available test suite passes
• Release notes updated
• GitHub Actions CI passes
• Test coverage to 100% (Codecov passes)

AI assistance

Codex assisted with test planning and independent review. I reproduced and evaluated each review finding, ran the tests and quality checks, and inspected the final diff.

[shixi-li]

CI/review follow-up: the four fork workflows (Tests, Wheels, i386, and Code Scanning) are all action_required and created no jobs, so no GitHub test has actually run. The PR is mergeable, the linked issue #852 remains open, and the local validation in the description is complete (654 tests passed with optional-dependency skips). Could a maintainer approve the workflows and review the opt-in compatibility change when convenient?

[d-v-b]

I'll have a look today

[d-v-b]

🤖 AI text below 🤖

Thanks for working on this! I ran a review of the PR (multiple independent review passes, with each finding adversarially verified by executing the code). The core feature unfortunately has some fundamental problems in real storage pipelines, plus a metadata-compatibility break. Details below, roughly in order of severity.

Correctness

1. Decode silently scrambles F-ordered data after serialization (fixedscaleoffset.py decode, the np.copyto branch)

With keep_dimensions=True, the same encoded content decodes to different values depending on whether it passed through serialization. Verified reproduction:

arr = np.arange(24, dtype='<f4').reshape((2, 3, 4), order='F')
enc = codec.encode(arr)              # F-contiguous (2,3,4)
out = np.empty((2, 3, 4), '<f4', order='C')
codec.decode(enc, out=out)           # correct (np.copyto branch)
codec.decode(enc.tobytes(order='A'), out=out)  # scrambled: [0,4,8,...] instead of [0,2,4,...]

Any compressor or store between encode and decode produces the second case: dec is 1-D, the dec.ndim > 1 guard skips the branch, and ndarray_copy reshapes the F-order byte stream in C order — silent data corruption for exactly the multi-order use case the PR targets.

2. keep_dimensions is inert once data is serialized

codec.decode(codec.encode(arr).tobytes()) returns shape (arr.size,), not arr.shape (verified). ensure_ndarray(bytes).view(astype) is always 1-D and no shape metadata is stored, so in a zarr filter chain — where decode always receives decompressed 1-D bytes — the option silently degrades to the old flattened behavior. The docstring's "Preserve the input array dimensions when encoding and decoding" only holds when encode's in-memory ndarray is passed directly back to decode in the same process.

3. get_config breaks forward compatibility for all users, even non-opt-in ones

get_config unconditionally emits 'keep_dimensions', so metadata written by this version (including the default False, whose data is byte-identical to old-format data) contains the key. Older numcodecs reconstructs codecs via Codec.from_config -> cls(**config) with no key filtering, and the released __init__ has no such kwarg → TypeError: unexpected keyword argument 'keep_dimensions'. Every existing release becomes unable to read newly-written arrays. Emitting the key only when True (as __repr__ already does) would confine the cost to opt-in users.

4. Output layout depends on out's concrete type and shape

The fast-path guard isinstance(out, np.ndarray) and out.shape == dec.shape and out.dtype == dec.dtype means (verified):

• a shaped memoryview (or other ndarray-like accepted by ndarray_copy) with identical shape/dtype falls through and receives coordinate-scrambled data — the exact bug the branch fixes for np.ndarray;
• a flat 1-D out receives F-memory-order bytes while a shaped out over equivalent memory receives logical C-layout: out1.tobytes() != out2.tobytes() for the same chunk. Callers following the historical "out is raw memory filled in encode order" convention get a different layout than callers passing shaped views.

5. Minor: 0-d input now yields a scalar — with keep_dimensions=True, encoding np.array(3.5, '<f4') returns a 0-d/scalar result (previously shape (1,)), so enc[0] raises IndexError.

Design

6. The np.copyto special case papers over a bug in the shared helper. The root cause is in compat.ndarray_copy: it flattens src with order='A' then reshapes in dst's contiguity order, scrambling any same-shape cross-memory-order copy. A ~2-line early path there (if src.shape == dst.shape after the dtype view: np.copyto(dst, src); return dst) would fix this for every codec and every ndarray-like destination, and decode could keep its single return ndarray_copy(dec, out). As written, the workaround is codec-private, partial (see 4), and its self.keep_dimensions / dec.ndim > 1 guard terms are redundant (when the flag is off, dec is always 1-D).

7. The flag persists an implementation detail into on-disk metadata. The transform is purely elementwise; the flatten exists only to "simplify implementation", and the stored bytes are identical either way. Quantize performs the same class of transform with no flatten and no flag. It's worth considering whether the right change is to just stop flattening (no config knob), plus the ndarray_copy fix — avoiding the schema widening and the compat break in (3) entirely.

Tests

8. The serialized-bytes paths are untested. No keep_dimensions=True codec is added to the module-level codecs list, so the generic check_encode_decode harness (which round-trips through bytes/bytearray/array.array and decode(out=...)) never exercises the flag on exactly the paths where items 1–2 misbehave. The new tests only feed encode's still-shaped ndarray directly back into decode — the one path that works — which is why CI is green. A test like codec.decode(codec.encode(arr).tobytes()) would surface the shape no-op immediately.

9. Efficiency note: with F-ordered input, encode now returns an F-contiguous chunk where it previously always returned a flat memory-order array; downstream consumers that C-flatten (e.g. tobytes(), ascontiguousarray) pay a transposing strided copy (measured ~7× a contiguous copy). If the feature's semantics are kept, this cost is inherent — but it's worth a deliberate, documented decision.

---

Overall: I don't think this should merge in its current form — the feature only functions in the direct in-memory encode→decode case, silently corrupts F-ordered data through any real pipeline, and the unconditional config key breaks reading new data with all existing releases. Happy to discuss the ndarray_copy-centered alternative, which seems like it would address #852 more robustly with a much smaller surface.

[shixi-li]

Thanks for the thorough review. I reproduced the findings and rewrote the PR at bbc3f4a around the smaller ndarray_copy-centered design:

• removed keep_dimensions entirely, including config/repr/schema changes
• stopped flattening non-scalar FixedScaleOffset inputs while preserving the historical 0-D shape
• fixed same-shape cross-order copies centrally for ndarray and shaped memoryview destinations
• added explicit serialized-byte, correctly shaped out, byte-compatibility, scalar, and config-boundary tests

The PR now states that raw serialized bytes remain 1-D because no shape/order metadata exists; encoded bytes are verified byte-identical to the legacy implementation for C and F inputs. The full locally available suite passes (656 passed, 32 optional-dependency skips), and Ruff check/format are clean. Could you please re-review the revised design when convenient?