Skip to content

fix: MultiCell loses contents when a growing axis resizes storage#1159

Draft
NJManganelli wants to merge 1 commit into
scikit-hep:developfrom
NJManganelli:fix_multicell_growth
Draft

fix: MultiCell loses contents when a growing axis resizes storage#1159
NJManganelli wants to merge 1 commit into
scikit-hep:developfrom
NJManganelli:fix_multicell_growth

Conversation

@NJManganelli

Copy link
Copy Markdown

Problem

Filling a MultiCell histogram that has a growing axis (e.g. StrCategory(growth=True))
silently zeroes everything filled before the axis grows. Any category that appears
after the first fill triggers a storage resize, and all previously accumulated cells
are lost. The only workaround today is to declare every category up front.

import boost_histogram as bh, numpy as np

def make(cats):
    return bh.Histogram(bh.axis.StrCategory(cats, growth=True),
                        bh.axis.Regular(3, 0, 3),
                        storage=bh.storage.MultiCell(2))

grown, pre = make([]), make(["a", "b", "c"])
for chan, xs, cells in [("a",[.5,1.5,2.5],[[1,10],[2,20],[3,30]]),
                        ("b",[.5,2.5],[[4,40],[5,50]]),
                        ("c",[1.5],[[6,60]])]:
    grown.fill(chan, np.asarray(xs), weight=np.asarray(cells, float))
    pre.fill(chan,  np.asarray(xs), weight=np.asarray(cells, float))

grown.view().sum()  # 13.0 + 130.0  -> wrong, most of it zeroed on resize
pre.view().sum()    # 28.0 + 280.0  -> correct

Cause

multi_cell_reference is a proxy reference (a span over a cell's elements). Its
element-wise assignment is provided only as a template operator=. A templated
assignment operator does not suppress the implicitly-declared copy-assignment
operator, so when both sides have the same type — multi_cell_reference = multi_cell_reference
overload resolution prefers the compiler-generated copy-assignment, which just rebinds
the span instead of writing through to the referenced cells.

boost::histogram relocates cells with *new = *old in
detail::storage_grower::apply when a growing axis enlarges the storage. With the
rebinding assignment the relocation is a no-op, so every growth discards the existing
contents. (Confirmed by reproducing against pristine boost::histogram develop: the
cell-relocation offsets are computed correctly, but *ns = x never copies.)

Fix

Provide an explicit write-through copy-assignment operator for multi_cell_reference,
and default the copy constructor to keep proxy rebind-on-copy semantics. This is the
same contract every other boost::histogram storage reference already honours.

Adds a regression test that fills a growing MultiCell histogram category by category
and checks it matches the pre-declared-categories result. Full test suite passes; clean
under -Wall -Wextra and clang-format.

Follow-up to #1008 (MultiCell storage).

🤖 Investigated and drafted with Claude Code; reviewed by @NJManganelli.

@github-actions github-actions Bot added the needs changelog Might need a changelog entry label Jul 14, 2026
@NJManganelli
NJManganelli marked this pull request as draft July 14, 2026 21:43
multi_cell_reference is a proxy reference whose element-wise assignment
was only provided as a template operator=. A templated assignment operator
never suppresses the implicitly declared copy-assignment operator, so for
`ref = ref` (both multi_cell_reference), the compiler-generated copy
assignment won overload resolution and merely rebound the span instead of
writing through to the referenced cells.

boost::histogram relocates cells with `*new = *old` in
detail::storage_grower::apply when a growing axis enlarges the storage, so
every growth silently zeroed all previously filled contents. Filling a
MultiCell histogram that has a growth axis (e.g. StrCategory) therefore lost
data whenever a new category appeared, unless all categories were declared
up front.

Provide an explicit write-through copy-assignment operator (and default the
copy constructor to keep proxy rebind-on-copy semantics). Add a regression
test that fills a growing MultiCell histogram category by category and checks
it matches the pre-declared-categories result.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@NJManganelli
NJManganelli force-pushed the fix_multicell_growth branch from 59904eb to 8aaa032 Compare July 15, 2026 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs changelog Might need a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant