feat: support axes growth in serialization#1074
Conversation
Signed-off-by: Henry Schreiner <henryfs@princeton.edu>
There was a problem hiding this comment.
Pull request overview
This pull request adds support for preserving the growth trait of axes during serialization and deserialization, addressing issue #1073. Previously, when a histogram with growable axes was serialized and then deserialized, the growth trait was lost, which prevented the deserialized histogram from growing its axes during fills.
Changes:
- Added serialization logic to record
growth=Truein thewriter_infosection when an axis has the growth trait enabled - Added deserialization logic to read and respect the
growthflag fromwriter_infowhen reconstructing axes - Added tests to verify growth trait preservation in round-trip serialization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/boost_histogram/serialization/_axis.py | Modified serialization functions to write growth trait to writer_info for all axis types that support growth (Regular, Integer, Variable, IntCategory, StrCategory), and modified deserialization to read and apply the growth parameter when constructing axes |
| tests/test_serialization_uhi.py | Added test for Integer axis with growth=True to verify round-trip preservation, and added assertion to existing test to verify growth trait is preserved |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_serialization_uhi.py
Outdated
| def test_round_trip_growth() -> None: | ||
| h = bh.Histogram( | ||
| bh.axis.Integer(0, 10, growth=True), | ||
| ) | ||
| h.fill([-1, 0, 0, 1, 20, 20, 20]) | ||
| data = to_uhi(h) | ||
| h2 = from_uhi(data) | ||
|
|
||
| assert h == h2 | ||
|
|
||
| assert isinstance(h2.axes[0], bh.axis.Integer) | ||
| assert h2.axes[0].traits.growth == h.axes[0].traits.growth |
There was a problem hiding this comment.
The test only covers Integer axes with growth. Consider adding tests for other axis types that support growth to ensure comprehensive coverage. Specifically, Regular, Variable, IntCategory, and StrCategory axes all support the growth trait and have serialization code added for it in this PR. Testing these would verify that the serialization/deserialization works correctly for all axis types that support growth.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
I knew that when writing it, I was just lazy. I am going to continue to be lazy and let AI write the expanded tests for me so I can focus on stuff that's more fun. :)
* Initial plan * test: expand growth serialization test to cover all axis types Co-authored-by: henryiii <4616906+henryiii@users.noreply.github.com> * style: pre-commit fixes --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: henryiii <4616906+henryiii@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Record
growth=Truein serialization, and respect it if present.Close #1073.