feat(propagator)!: :sparkles: name the evaluation functionals, and follow an initial-operator re-weight by robertodr · Pull Request #251 · Algorithmiq/monoprop

robertodr · 2026-08-20T06:49:27Z

🤖 AI text below 🤖

Summary

make_functional_ returned an anonymous std::function whose twelve-item capture list carried its
ownership and lifetime rules in five comment paragraphs, because the object had no name and no
declaration. This replaces it with two named function-object classes over one named plan, so those
rules become a member list a reader can check.

Reading the two validity checks closely enough to move them turned up three defects, all fixed here:

The graph-layer check could never fire for a pared functional. expected_layers was read off
the live graph, then compared against the owned pared graph, whose layer count is fixed at
construction and equal to it by construction. A later build_graph therefore passed both checks,
and the call went on to read the cosine callbacks' raw pointers into an inverted index that had
been grown or rebuilt — undefined behaviour.
set_parameter_mapping moved neither check. It relabels the layers in place, so a functional
built before the call kept the old labels and returned a wrong number with no message.
The epoch check dereferenced the propagator to decide whether it was safe to dereference the
propagator. A call after the propagator's destruction read freed memory inside the check meant
to prevent exactly that. The Python front end hid it (the wrapper captured self); the bound core
class had no keep_alive, and a test used that raw path.

One detail::FunctionalControl block — a structure revision, an alive flag, and the name of the last
structural change — replaces both counters. Every mutating method bumps it, which is now pinned by a
build-time assertion: MonomialPropagator::num_mutating_methods states the roster and
cpp/tests/functional_validity.cpp static_asserts its row count against it, so a new mutator
breaks the build until its effect on a live functional is recorded.

On top of that, one documented behaviour change: update_initial_operator no longer invalidates a
live functional. A re-weight moves no structure — MPOperator::update_initial_operator cannot add
a store row, so the store, the inverted index and the graph all stay put — and the only state it does
move is a coefficient vector and the core term. Those travel together as a detail::OperatorWeights
set published into the control block, which a call reads with one atomic load, so op and
core_term can never come from two different re-weights. The price is that a functional is a live
view of the weights: two calls with the same parameters give two answers across a re-weight, and a
caller who needs a frozen value must build the functional again. Two cases still throw — a
Schrödinger functional with a pare_threshold, whose keep-set was thresholded from the very
coefficients the re-weight replaced, and a re-weight that fails part-way.

Landed as seven commits, one per stage of the plan, each green on its own.

Changes

cpp/tests/functional_validity.cpp (first commit) — the mutation table: one row per public
mutating method, recording what a functional built before that call does when called after it,
asserted for the value and the gradient functional, with and without a pare threshold, in both
pictures. tests/test_parameter_validation.py::TestFunctionalValidityTable mirrors it through the
Python front end over monoprop_PARTITIONS=off and =auto.
cpp/include/monoprop/Functional.h — ExpectationValueFunctional<NumModes> and
ExpectationValueAndGradientFunctional<NumModes>, each a handle on one shared
detail::FunctionalPlan<NumModes>. The plan's std::variant carries the single-partition shape and
the partition-facade shape, so both paths have one public type, and the value and gradient
functional over one snapshot share one plan. Bound with nb::keep_alive<0, 1>.
cpp/monoprop/detail/functional/Control.h — the shared validity block, plus the published
OperatorWeights.
Validation.h/.cpp — validate_expected_graph_layers and validate_expected_initial_operator
give way to validate_functional_state and validate_weight_refresh. Still
StaleFunctionalGraph, so the promised RuntimeError is unchanged.
The plan owns its layers. cos holds a raw CosMask pointer per layer, so a later
append_layer, slice_graph or maybe_compact_layers must not be able to move or free them. The
copy is one pointer per layer for a normally built graph.
An operator-layout backstop — the store pointer and the inverted-index row count, re-derived and
compared before any use of that index, so a mutation that forgets its bump_structure_ reports
staleness instead of folding a rebuilt index through a pointer to the old one.
follows_weights on both classes, carried through onto the callables the front-end factories
return, so a caller holding a functional can read the contract off it.
Docs — the full mutation table on the evaluation page, linked to the executable one; the
re-weight rule and its price in the docstrings of both factories and of update_initial_operator
(base class and both front ends); AGENTS.md records where the weights live and who may publish
them, next to the bump_structure_ rule it qualifies.

Measured, since the plan expected the module to grow: it shrank by 16 KiB (2 406 200 → 2 389 816
bytes), and a call got 2–3% faster. Losing a layer of type erasure more than pays for two extra
classes per mode width.

Verification beyond the suites: the Stage 3 layer-ownership change was proved bit-identical to Stage 2
across eight configurations via hex-float fingerprints, and the weight refresh is asserted bit for
bit rather than to a tolerance — a re-weighted propagator's functional returns exactly what a
functional over a propagator built with those coefficients returns, for value and gradient, pared and
unpared, at partitions=1 and the default count, under MPI at 2 and 4 ranks. An ASan+UBSan run over
232 cases is clean; it is also what caught defect 3's fix being incomplete in an intermediate commit
(fix(propagator): check the alive flag before the operator backstop) — the ordinary build read
plausible freed memory and passed all 213 tests.

Two notes for the reviewer:

A rejected re-weight invalidates live functionals, including one rejected before anything was
written, because apply_initial_operator_ can throw with the core term already written and a facade
can have applied some partitions. Conservative, and no worse than before (the old code bumped
unconditionally). Making the single-partition path transactional — deferring the core_term_ write
until after MPOperator::update_initial_operator commits — would make the check exact; it changes
failure-path semantics, so it is not in this PR.
No changelog entry: this repository has no changelog file, and .github/release.yml generates
release notes from PR titles. The feat! commit title and body carry the behaviour change instead.

Checklist

Tests added or updated to cover the changes
Documentation updated (docstrings, docs/, CONTRIBUTING.md) if needed
CHANGELOG / release notes updated if applicable — no changelog file exists; see the note above

AI/LLM disclosure

I used the following tool to help write this PR description: Claude Code (claude-opus-5)
I used the following tool to generate or modify code: Claude Code (claude-opus-5), noted inline
by the Assisted-by: trailer on every commit

Important

By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.

🤖 Generated with Claude Code

Stage 0 of the function-object plan: write down today's behaviour before changing any of it. One table row per public mutating method of MonomialPropagator, asserted for the value and the gradient functional, with and without a pare threshold, in both pictures. Two rows record defects rather than intent, each with the stage that fixes it: - a pared plan owns its layers, so its layer count cannot move and the live-graph check never fires after build_graph or an in-place contract_partially; - set_parameter_mapping relabels in place, so neither the layer count nor the initial-operator epoch moves and the plan keeps replaying the old labels. Both get a witness test that shows the propagator's own answer *did* move while the functional's did not, so the rows cannot pass vacuously. num_mutating_methods pins the roster on the class and the table static_asserts against it, so a new mutator cannot land without recording what it does to a live functional. Assisted-by: ClaudeCode:claude-opus-5

Stage 1 of the function-object plan. make_functional_ returned an anonymous std::function whose twelve-item capture list carried the ownership and lifetime rules in five comment paragraphs, because the object had no declaration to put them on. Those captures are now the fields of detail::FunctionalPlan<NumModes>: the propagator snapshot a call replays, plus the checks that say the snapshot is still that propagator's. The plan's std::variant carries the single-partition shape and the facade shape, so both paths have one public type, and the two functional kinds over one snapshot share one plan -- a facade now fans out once instead of once per kind. expectation_value_functional and expectation_value_and_gradient_functional return ExpectationValueFunctional<NumModes> and ExpectationValueAndGradientFunctional<NumModes>. Both report num_params. Every C++ call site used auto, so none needed an edit. The bound classes carry nb::keep_alive<0, 1> on their factories, which closes the last of the three defects the plan found: the raw core class let a functional outlive the propagator whose inverted index it reads, and only the Python front-end's wrapper lambda was hiding it. No behaviour change. Both checks keep the same arithmetic, and the mutation table is unchanged. The module got 16 KiB smaller (2 406 200 -> 2 389 816 bytes): losing one layer of type erasure more than pays for two more classes per mode width. A functional call is 2-3% faster (energy 8.2 -> 8.0 us, energy+gradient 19.5 -> 18.8 us on a 964-term, 60-layer problem). Assisted-by: ClaudeCode:claude-opus-5

Stage 2 of the function-object plan. Two counters guarded a functional before: the graph's layer count and an initial-operator epoch. Between them they missed three mutations and read one raw pointer into the propagator they were supposed to protect. detail::FunctionalControl replaces both. A propagator shares one block with every plan it makes: a structure revision, an alive flag, and the name of the method that last moved the structure. bump_structure_ is called from build_graph, propagate, an inplace contract_partially and set_parameter_mapping -- and deliberately not from the update_setting_ family, whose atols, cutoff, cutoff type and basis change gate the next build and touch nothing a plan holds. What this closes: - set_parameter_mapping relabels in place, so the layer count and the epoch both held and a functional kept answering for the old labels. It now throws, naming the method. - propagate leaves the layer count at zero, so nothing saw the re-evolved operator. It now throws. - a pared plan owns its layers, so its layer count could never move; both build_graph and an inplace contract_partially slipped past it. The revision does not care which shape the plan has. - a functional that outlives its propagator reports the destruction instead of folding a dangling inverted-index pointer. A facade's plan checks the facade's own block, since the partition group goes with it. As a backstop for the still-borrowed inverted index, a plan also re-derives the operator's store pointer and index row count and compares them before it reads the index. That check comes from the data, so a future mutator that forgets its bump reports staleness rather than a wrong number. validate_expected_graph_layers and validate_expected_initial_operator are gone, replaced by validate_functional_state. Both were monoprop_EXPORT with no callers outside the .inl. Assisted-by: ClaudeCode:claude-opus-5

The layout backstop reads the propagator's operator store through a borrowed pointer, and it was written as one field of the aggregate passed to validate_functional_state. Every argument is evaluated before the callee runs, so on a destroyed propagator the read happened before the alive flag could stop it -- a heap-use-after-free inside the check whose job is to prevent exactly that. Aliveness is now settled in validate() and a dead propagator drops out of the argument list, leaving the control block, which is shared and outlives the propagator, as the only thing read. Found by running the C++ suite under the address and undefined-behaviour sanitisers; the ordinary build read plausible freed memory and passed. Assisted-by: ClaudeCode:claude-opus-5

Stage 3 of the function-object plan. Without a pare threshold the plan aliased the propagator's graph_ through a shared_ptr with an empty owner block, while its cosine callbacks held one raw CosMask pointer per layer -- pointers a later append_layer, slice_graph or maybe_compact_layers could move or free. The pared path already owned its layers, because pare_graph builds a new graph. Both paths now own their layers, so those pointers cannot dangle. The copy is cheap: a Layer is a shared_ptr to an immutable core plus an optional CosMask, and MPGraph::append never stores a cosine set, so copying a normally-built graph is one pointer copy per layer. Every number is bit-identical to stage 2 -- checked as hex floats across both pictures, both partition settings and both pare settings, for the value and the gradient. The C++ suite is clean under the address and undefined-behaviour sanitisers. Assisted-by: ClaudeCode:claude-opus-5

`update_initial_operator` used to invalidate every live functional: a call after it threw `StaleFunctionalGraph` rather than answer. It no longer does. A re-weight moves no structure -- `MPOperator::update_initial_operator` cannot add a store row, so the store, the inverted index and the graph all stay put -- and the only state it does move is a coefficient vector and the core term. Those two now travel together as a `detail::OperatorWeights` set, published into the shared `FunctionalControl` block instead of bumping the structure revision. A call reads the set with one atomic load, so `op` and `core_term` cannot come from two re-weights, and a facade publishes through `for_each_partition_` so every partition has published before the call returns. Two cases still throw: - A Schrodinger functional built with a `pare_threshold`. Its keep-set was thresholded from the very coefficients the re-weight replaced, so the pared graph it holds is not the graph the new coefficients ask for. Heisenberg pares the state, which a re-weight leaves alone, so it follows exactly. - A re-weight that fails part-way. `apply_initial_operator_` can throw with the core term already written, and a facade can have applied some partitions, so the failure path bumps the revision -- invalidating a functional that did not need it costs a rebuild, answering from a half-written operator costs a wrong number. The price of the new rule is that a functional is a live view of the weights: two calls with the same parameters give two answers across a re-weight, and a caller who needs a frozen value must build the functional again after the last one. The docstrings say so. Verified bit-for-bit rather than to a tolerance: a re-weighted propagator's functional returns exactly what a functional over a propagator built with those coefficients returns -- value and gradient, pared and unpared, one partition and the default count, and under MPI at 2 and 4 ranks. Assisted-by: ClaudeCode:claude-opus-5

…tional report it The re-weight rule was only in the commit that changed it. It now sits where a reader meets a functional: - The evaluation page carries the whole mutation table -- one row per public mutating method, saying what a call afterwards does -- and points at `cpp/tests/functional_validity.cpp`, which asserts the same rows. The initialisation page's re-weight section links to it. - `AGENTS.md` records where the weights live and who may publish them, next to the `bump_structure_` rule it qualifies. Both functional classes gain a read-only `follows_weights`, so a caller holding a functional and not its propagator can see which case it is instead of re-deriving it from the picture and the pare threshold. The front-end factories carry the attribute through onto the callable they return: the engine-level functional is not part of the public surface, so leaving it only there would put the contract out of reach of every user of `MajoranaPropagator` and `PauliPropagator`. Assisted-by: ClaudeCode:claude-opus-5

github-actions · 2026-08-20T06:54:56Z

Docs preview: https://pr-251.monoprop-docs.pages.dev

codecov · 2026-08-20T06:57:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.78%. Comparing base (412315e) to head (912acf0).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #251      +/-   ##
==========================================
+ Coverage   97.70%   97.78%   +0.08%     
==========================================
  Files          14       14              
  Lines         742      769      +27     
  Branches       98       98              
==========================================
+ Hits          725      752      +27     
  Misses         12       12              
  Partials        5        5

Flag	Coverage Δ
cpp	`97.78% <100.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

… fixture and the bound functional The rule that decides whether a functional may follow a re-weight was written twice, in opposite polarity: FunctionalPlan::follows_weights() returned !pared_from_operator, and validate_weight_refresh threw on it. resolve_weights now passes its own follows_weights(), so the property advertised out to Python and the check that enforces it are the same expression. The front end returned closures with engine attributes hand-copied onto them, a list that had already lost num_params. _BoundFunctional forwards by __getattr__ instead, so a property added on the C++ side surfaces without an edit here. Also: publish the initial-operator weights only when a set has already been handed out, since publishing copies the whole coefficient vector; read the facade's parameter axis off a child plan rather than rebuilding partition 0's gate arrays; fold the re-weighted propagator fixture into make_propagator with one named coefficient; drop a roster assertion that compared the table to a copy of itself, and pin the row count on the static_assert that is the real gate. Assisted-by: ClaudeCode:claude-opus-5

adamglos92 · 2026-08-20T07:59:40Z

+        assert after == pytest.approx(mp.expval(parameters))
+        assert after == _value(getattr(mp, functional_name)()(parameters))


question: would the test be cleaner if the reference value would be computed with a fresh mbs?

no, that would defeat the point of the changes in this PR. Which is to ensure that functionals taken from a single propagator stay valid, under certain conditions. Basically, hardening what was done in #225

adamglos92 · 2026-08-20T08:00:14Z

        ],
    )
-    def test_functional_invalidated_after_initial_operator_update(
+    def test_functional_follows_initial_operator_update(


perhaps this question should also compare gradients? for some reason it only checks expectation values

adamglos92 · 2026-08-20T08:10:11Z

+        assert after == _value(getattr(mp, functional_name)()(parameters))
+
+
+class TestFunctionalValidityTable:


I'm completely lost with this tests

this PR adds ways to keep a functional valid when a limited set changes are made to the parent propagator. This test checks that this limited set of changes produces the desired behavior.

Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com> Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>

the latter is not implemented in LLVM libc++

sonarqubecloud · 2026-08-20T17:48:46Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
78.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

robertodr added 7 commits August 20, 2026 06:40

robertodr requested review from adamglos92, diagonal-hamiltonian, fpietra and ludmilaasb as code owners August 20, 2026 06:49

github-actions Bot added documentation Improvements or additions to documentation python cpp labels Aug 20, 2026

robertodr removed the request for review from adamglos92 August 20, 2026 06:52

adamglos92 reviewed Aug 20, 2026

View reviewed changes