Fix EnumSerializer.write to accept numpy scalars from NDArray of records#285
Fix EnumSerializer.write to accept numpy scalars from NDArray of records#285SAY-5 wants to merge 1 commit into
Conversation
2a9f25d to
6f48815
Compare
There was a problem hiding this comment.
This is not the right way to test this.
The test model should be updated such that it reproduces the issue. Then regression tests can be added to test_generated_types.py and test_protocol_roundtrip.py. Then, an equivalent test should be added for C++ and MATLAB to ensure no similar bugs exist.
|
Understood, that's a much bigger refactor than I have bandwidth for; happy to close in favour of a more thorough fix. |
|
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. |
|
Closing per maintainer feedback, will resubmit with the proper test model approach. |
|
Reopened. Reworking per your feedback: I'll update the test model to reproduce the issue, add regression tests in |
6f48815 to
58785a3
Compare
|
Reworked as discussed. Added |
There was a problem hiding this comment.
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
.valueinEnumSerializer.writeonly when input is anEnum; pass numpy scalars through. - Make
RecordSerializer.read_numpydispatch toread_numpyon Enum/Record field serializers to keep numpy-compatible values. - Add
recArray: RecordWithEnums[]step to theEnumstest 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.
| # Enum and nested-record fields must be read via read_numpy so they | ||
| # yield numpy-assignable values rather than Enum or record objects. | ||
| return cast( | ||
| np.void, | ||
| tuple( | ||
| serializer.read_numpy(stream) | ||
| if isinstance(serializer, (EnumSerializer, RecordSerializer)) | ||
| else serializer.read(stream) |
| int_value = value.value if isinstance(value, Enum) else value | ||
| self._integer_serializer.write(stream, int_value) |
Closes #284.
EnumSerializer.writeassumed itsvalueargument was always a PythonEnum, but when called fromRecordSerializer._writeon the numpy write path (where a record is iterated asvalue['fieldname']from anp.voidscalar), it receives a barenp.integerand crashes withAttributeError: 'numpy.uint64' object has no attribute 'value'. This affected anyT[]whose element type was a record containing one or more enum fields.Fix unwraps
.valueonly whenvalueis anEnum; numpy scalars (which_integer_serializer.writealready accepts) are passed through. Addspython/tests/test_enum_in_ndarray.pycovering: the original NDArray-of-record-with-enums reproducer, the bare numpy-scalar path directly throughEnumSerializer.write, and the existing Python-Enum path (to guard against regression).