Fix NumPy field dispatch and record-array roundtrips

[Sai Asish Y (SAY-5)]

Closes #284.

Fix the Python NumPy record path so fields use their NumPy serializer methods while ordinary record values retain object-path dispatch. This covers enum fields, nested records, fixed and variable-length vectors, optionals, and strings inside arrays of records.

Variable-length vector slots now accept a Python list or a one-dimensional NumPy array, dispatch each through the corresponding element path, and preserve decoded lists for subsequent serialization. Nested Time dtypes and the NDJSON time converter use the existing nanosecond contract. The tests include a nonzero optional time and an absent value.

Keep enum cases in Enums. Move the fixed-vector, optional, and string record-array steps to DynamicNDArrays, reuse its existing variable-length-record step, and regenerate the C++, Python, and MATLAB outputs. Matching roundtrip cases remain in all three language suites; Python also checks generated dtypes and real decode-then-reencode behavior.

Local validation on macOS ARM64:

• C++20: 155 native tests passed, with HDF5 and NDJSON enabled.
• Python: 112 tests passed, including 120 calls to the actual C++ translator for binary/NDJSON conversions.
• Go tooling: 182 top-level tests and 253 subtests passed.
• Pyright 1.1.409: 42 files, no errors or warnings; the repository pins 1.1.406.

These are staged checks, not a complete just validate run. MATLAB was not run. A genuine C++17 attempt with xtensor 0.25 failed on a Clang template-specialization ambiguity; the successful build uses the repository's pinned xtensor 0.27.1, which requires C++20. The local native environment also uses HDF5 2.2.0 instead of the pinned 1.14.6. No C++17 or hosted CI success is claimed.

AI assistance was used for this follow-up implementation, review, and local validation.

[Sai Asish Y (SAY-5)]

Understood, that's a much bigger refactor than I have bandwidth for; happy to close in favour of a more thorough fix.

[Sai Asish Y (SAY-5)]

Understood, the proper test should reproduce the bug through the YAML test model and have regression coverage in the generated-types and protocol-roundtrip suites for Python, C++, and MATLAB. That's a substantially different scope from this PR, so I'll close this and open a follow-up once the test model and codegen changes are ready.

[Sai Asish Y (SAY-5)]

Closing per maintainer feedback, will resubmit with the proper test model approach.

[Sai Asish Y (SAY-5)]

Reopened. Reworking per your feedback: I'll update the test model to reproduce the issue, add regression tests in test_generated_types.py and test_protocol_roundtrip.py, and add equivalent coverage for C++ and MATLAB to confirm no similar bugs exist there. Will push and re-ping.

[Sai Asish Y (SAY-5)]

Reworked as discussed. Added recArray: RecordWithEnums[] to the Enums protocol in the test model and regenerated all backends, then added regression coverage to the Python, C++, and MATLAB roundtrip suites plus a dtype assertion in test_generated_types.py. The roundtrip exposed a matching read-path bug, so RecordSerializer.read_numpy now reads enum and nested-record fields via read_numpy so they are numpy-assignable. Removed the standalone test file. Local Python (100 passed), C++ (155 passed) and Go tooling suites are green; CI is waiting on workflow approval.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes a crash in EnumSerializer.write when called from the numpy-record write path where enum fields arrive as bare numpy scalars (rather than Python Enum instances). Also adjusts RecordSerializer.read_numpy so enum and nested-record fields are read via read_numpy, producing numpy-assignable values. Adds a new recArray step to the Enums test protocol and corresponding generated code across C++, Python, and MATLAB targets, plus roundtrip tests.

Changes:

• Unwrap .value in EnumSerializer.write only when input is an Enum; pass numpy scalars through.
• Make RecordSerializer.read_numpy dispatch to read_numpy on Enum/Record field serializers to keep numpy-compatible values.
• Add recArray: RecordWithEnums[] step to the Enums test protocol and regenerate all targets/tests.

Reviewed changes

Copilot reviewed 9 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tooling/internal/python/static_files/_binary.py	Core fix: tolerate numpy scalars in EnumSerializer.write; route Enum/Record subfields via read_numpy in RecordSerializer.read_numpy.
python/tests/test_protocol_roundtrip.py	Adds NDArray-of-records-with-enums roundtrip exercise.
python/tests/test_generated_types.py	Asserts dtype layout for RecordWithEnums.
python/test_model/protocols.py	Regenerated: adds write_rec_array/read_rec_array and updated state machine.
python/test_model/binary.py	Regenerated binary impls for new recArray step.
python/test_model/ndjson.py	Regenerated NDJSON impls for new recArray step.
models/test/unittests.yml	Adds recArray field to the Enums protocol.
matlab/test/RoundTripTest.m	Adds matching MATLAB roundtrip data for recArray.
matlab/generated/+test_model/EnumsWriterBase.m, EnumsReaderBase.m	Regenerated MATLAB base classes with new state.
matlab/generated/+test_model/+binary/EnumsWriter.m, EnumsReader.m	Regenerated MATLAB binary impls.
matlab/generated/+test_model/+testing/*.m	Regenerated MATLAB testing mocks.
cpp/test/roundtrip_test.cc	Adds matching C++ roundtrip data for recArray.
cpp/test/generated/protocols.{h,cc}	Regenerated base protocol with new step and state.
cpp/test/generated/{binary,ndjson,hdf5}/protocols.{h,cc}	Regenerated backend impls.
cpp/test/generated/mocks.cc	Regenerated mock with new expectation method.
cpp/test/generated/model.json	Regenerated schema.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Sai Asish Y (SAY-5)]

Pinging in case this got buried: the rework pushed on 2026-05-19 adds recArray: RecordWithEnums[] to the test model, adds regression tests in test_generated_types.py, test_protocol_roundtrip.py, and the C++ and MATLAB roundtrip suites. CI is all green. Ready for re-review.

[Sai Asish Y (SAY-5)]

Good call on the fragility, the narrow isinstance check would silently drop optionals and the like. 82221ad just reads every field through read_numpy now, and I confirmed locally that optional, string, and date fields inside a record-in-NDArray all roundtrip back assignable (the old path raised a TypeError on a None optional).

[Joe Naegele (naegelejd)]

Thanks for the rework. Two things before this can land:

The latest commit (82221ad) introduces a regression.
RecordSerializer.read_numpy now calls serializer.read_numpy(stream) for every field, but FixedVectorSerializer.read_numpy (and write_numpy) explicitly raise NotImplementedError("Internal error: expected this to be a subarray"). That assumption no longer holds once a record-with-fixed-vector field is read via the numpy record path. The current test model doesn’t exercise this, so CI won’t catch it.

There is still an unfixed write-path bug.
Generated RecordSerializer.write_numpy calls self._write(...), which dispatches each field through object-path write(...) instead of numpy-path write_numpy(...). The enum crash was just the loud symptom; optionals are also wrong on this path (silent corruption risk).

Required changes

1. In RecordSerializer, add _write_numpy(self, stream, *values) that dispatches per field to serializer.write_numpy(...). Keep _write(...) for object-path dispatch.
2. Keep read_numpy dispatching to per-field serializer.read_numpy(...), but make FixedVectorSerializer.write_numpy/read_numpy actually round-trip in this context (instead of raising).
3. In tooling/internal/python/binary/binary.go, update generated record serializers so write_numpy(...) calls _write_numpy(...) (not _write(...)), then regenerate outputs.
4. Revert the EnumSerializer.write numpy-scalar fallback; once dispatch is correct, numpy values should only hit write_numpy(...).

Missing test coverage (must be added)

Please extend the test model with NDArray-of-record steps and add regression coverage in Python/C++/MATLAB for:

• RecordWithFixedVectors[] (catches the current regression),
• RecordWithOptionalFields[] (catches write-path dispatch bug),
• RecordWithVlens[],
• RecordWithStrings[].

Also add/update Python assertions in:

• python/tests/test_protocol_roundtrip.py
• python/tests/test_generated_types.py

RecordWithEnums[] alone is not enough to prove the record numpy path is correct across field serializers.

[Sai Asish Y (SAY-5)]

You're right, and thanks for the careful read. Calling read_numpy on every field does break FixedVectorSerializer, which still raises NotImplementedError outside the subarray path, and the current test model doesn't exercise a record-with-fixed-vector inside an NDArray so CI stays green on a path that's actually broken. The write side has the matching _write/write_numpy dispatch bug you describe too.

I'll redo this along the lines you laid out: add a _write_numpy to RecordSerializer for per-field numpy dispatch, make FixedVectorSerializer.read_numpy/write_numpy round-trip in the record context instead of raising, update the generated serializers in tooling/internal/python/binary/binary.go to call _write_numpy, drop the EnumSerializer.write scalar fallback once dispatch is correct, and regenerate all backends. I'll also extend the test model with NDArray-of-record steps for RecordWithFixedVectors, RecordWithOptionalFields, RecordWithVlens, and RecordWithStrings, with roundtrip coverage in Python/C++/MATLAB and assertions in test_protocol_roundtrip.py and test_generated_types.py. Will push once it's all green locally.

[Sai Asish Y (SAY-5)]

Thanks, that's a clear breakdown. You're right that 82221ad over-generalized the read path and the write path was never actually fixed. I'll restructure it as you laid out: a separate _write_numpy dispatch, real round-trip for FixedVectorSerializer in the numpy context, the binary.go codegen change so write_numpy calls _write_numpy, and drop the EnumSerializer fallback once dispatch is correct. I'll also extend the test model with the NDArray-of-record steps and add the fixed-vector/optional/vlen/string regression coverage across Python, C++, and MATLAB before pushing again.

[Sai Asish Y (SAY-5)]

Pushed the rework. RecordSerializer now has a separate _write_numpy that dispatches each field through write_numpy (binary.go emits write_numpy -> _write_numpy), FixedVectorSerializer.read_numpy/write_numpy round-trip the subarray instead of raising, and the EnumSerializer.write scalar fallback is gone now that dispatch is correct. I extended the Enums protocol with RecordWithFixedVectors[], RecordWithOptionalFields[], RecordWithVlens[] and RecordWithStrings[] steps, regenerated all backends, and added matching roundtrip coverage plus dtype assertions in test_protocol_roundtrip.py/test_generated_types.py for Python/C++/MATLAB. The Python self-roundtrip passes locally for all four new record types; the cross-format leg needs the C++ translator which builds in CI.

[Joe Naegele (naegelejd)]

You are not running the full test suite locally before pushing. Do not waste our time. If not all tests pass, your PR is not ready for review. You made the changes I suggested, but did not verify that they are sufficient and complete. I understand if installing MATLAB is a blocker, but building the C++ code and running the inter-language protocol roundtrip tests is mandatory.

I am happy to accept PRs from an "AI agent" if and only if it does not waste our time.

[Joe Naegele (naegelejd)]

The fields that are unrelated to Enums should be added to a different Protocol. The Enums protocol is for testing types related to Enums.

The DynamicNDArrays protocol already has a field recordWithVlensArray: RecordWithVlens[]. Use your best judgement to move the rest of the fields to a more appropriate protocol in the test model.

[Sai Asish Y (SAY-5)]

Understood on the verification bar, and you are right that the C++17/C++20 validation has to be green before this is review-ready. The earlier pushes leaned on the Python tests and the generated-code diff without a full local C++ build, which is on me.

I am working through it properly this time: regenerating, building the generated C++ under both C++17 and C++20, and running the inter-language roundtrip suite locally before I push again. I will also move the non-Enum fields off the Enums protocol per your inline note (RecordWithVlens-style fields onto DynamicNDArrays, the rest onto the closest-matching protocol). MATLAB I cannot run locally, so I will flag that target explicitly rather than claim it passed.

I would rather hold the next push until the C++ validation is actually green on my machine than send another unverified revision. Will report back once it builds and the roundtrips pass.

[Sai Asish Y (SAY-5)]

You're right, the last push wasn't fully green and I shouldn't have sent it in that state. I re-read the CI: C++ tests pass (155/155), but just python-test has two roundtrip failures:

• test_subarrays_in_records[binary] -> _binary.py:1042 ValueError: Expected a list, got numpy.ndarray. Same root cause as this PR but in the vlen-subarray write path, which my change didn't cover.
• test_enums[ndjson] -> _ndjson.py:1023 dtype mismatch <m8 vs <m8[ns], coming from the optional-time field in the restructured Enums protocol.

I'll hold off pushing until I've run the full just test (C++ build + cross-language roundtrip) locally and it's green end to end, and I'll fold in the model-restructuring you asked for (moving the non-enum fields out of Enums into DynamicNDArrays/a more appropriate protocol) in the same pass so the regenerated code and tests land together. Sorry for the back and forth.

[Sai Asish Y (SAY-5)]

The non-enum cases are now in DynamicNDArrays, reusing its existing recordWithVlensArray step. Enums retains recArray, and the corresponding C++, Python, and MATLAB generated code and test calls have been updated together.

The two outstanding Python failures were reproduced locally. The variable-length vector serializer now handles one-dimensional NumPy arrays and preserves decoded lists for another real serialization. Nested optional time dtypes use nanoseconds, with present and absent values covered. The tests also reject invalid vector shapes before writing a length prefix.

I built and ran the actual native and inter-language tests: 155 C++20 tests passed, and 112 Python tests passed with 120 calls to the real binary/NDJSON translator. The Go tooling suite passed 182 top-level tests plus 253 subtests. Pyright 1.1.409 checked 42 files without errors or warnings.

The remaining validation limits are explicit: MATLAB and the full just validate recipe were not run. A genuine C++17 build with xtensor 0.25 failed in dependency headers on this Clang version; the successful build uses the pinned xtensor 0.27.1, whose target requires C++20. Local HDF5 is 2.2.0 rather than 1.14.6. I am not claiming C++17 or hosted CI is green. This follow-up used AI assistance.