refactor(c++)!: :art: de-tangle picture decisions by robertodr · Pull Request #242 · Algorithmiq/monoprop

robertodr · 2026-08-18T11:20:10Z

🤖 AI text below 🤖

Summary

Two related refactors, no behaviour change. Both are verified bit-identical against main on every
floating-point result the pictures reach.

1. State each picture rule once, in a stateless policy. The Heisenberg/Schrödinger choice was one
bool schrodinger_, derived from std::optional<unsigned int> schrodinger_cutoff, and read at 17
decision points inside MonomialPropagator. The gate-order rule alone was written four times, and
contract_partially held two near-identical ~28-line arms. HeisenbergPicture and
SchrodingerPicture are now sibling policy models over shared primitives, exactly as
MajoranaAlgebra/PauliAlgebra already were, bound to the runtime value once per public entry point
by with_picture. The whole private layer is written against one picture and never re-tests which one
it is in.

The choice also becomes visible in the type system: the constructor takes a
PictureSpec = std::variant<Heisenberg, Schrodinger>, so only a Schrödinger run carries a state
cutoff. That deletes the "an empty optional means Heisenberg" encoding, along with the dead
schrodinger_cutoff.value_or(cutoff + 2) fallback the old spelling made unreachable.

2. Take the picture out of MPGraph, then simplify what was left. MPGraph carried a Picture
and branched on it, which put a simulation concept inside a container. It now takes an ArrivalOrder
naming the slope of the optimizer slots a build hands to append(). Following that through surfaced
five further problems in how layers were stored and addressed, each fixed in its own commit:

The partial-slice machinery was dead. contract_partially was the only production caller of
slice_graph/slice_view, and it always passed key == layers(). Both meant one view and one
drain, so front_offset_, maybe_compact_layers, the 4096 compaction threshold and the erase arm
had no production reachability at all. Gone, with the tests that were their only reachers.
A slice's metadata could contradict its own layout. slice_graph stamped its source's growth
onto a slice whose layer order disagreed with it, guarded only by a prose warning. It is now
unrepresentable: MPGraph has one constructor.
MPGraphView reimplemented std::span. Its (container pointer, base, count) trio is one
std::span member; sizeof drops 40 → 24 bytes on a type copied by value into an EvalRequest on
every evaluation, and the class stops naming std::vector. The header now also records why this is
not a std::ranges adaptor: consumers index it rather than iterate it, one type must serve both
directions and std::ranges::any_view is C++26, and the type sits in exported signatures.
n - 1 - x was open-coded at several sites, each re-deriving the storage invariant from a comment.
They all call slot_of_layer() now, which is its own inverse, with two static_asserts pinning it. It
and the bounds-checked checked_layer_offset() live below MPGraph, so a graph and the views over it
share one spelling of the mapping and one diagnostic instead of a copy each.
pare_graph took the picture to answer a question the graph already knew. It read a
bool schrodinger to choose which end to sweep from. The sweep in fact starts at the last gate the
build applied under either picture — Heisenberg reaches it via layer n-1 (slot 0, its last
simulation step), Schrödinger via layer 0 (slot n-1, also its last) — because the paring seed is
the result of the whole evolution, and reachability runs backwards from a result through the circuit
that produced it. pare_graph now takes no direction at all and asks
MPGraph::layer_of_unbuild_step(). Nothing previously guarded the direction beyond one hand-passed
argument in a test; a new case asserts both arrival orders yield the same sweep sequence.
An ascending-slot build inserted at layers_.begin() once per gate, so appending was O(n) each
and the build was quadratic. layers_ now only ever push_backs and get_layer() maps a layer
index onto arrival order instead. Measured on one machine at 20k/40k/80k layers:
177 / 716 / 2863 ms before, 4.6 / 7.5 / 14.7 ms after — quadratic to linear, ~195× at 80k.

3. A cleanup pass over the result. A review of the finished branch for reuse, duplication, wasted
work and misplaced logic turned up a further set, all in the last commit and all bit-identical:

Two policy constants were not independent facts. is_schrodinger restated picture, and
contract_phase was apply_sign under a second name — both are the sign an applied angle carries,
and contract_partially replays the very angles the build applied, so they cannot differ.
contract_reverse is likewise arrival_order == DescendingSlot, because map_params must invert
gate_slot. The PicturePolicy concept shape-checks exactly these constants and none of the
behavioural members, so a picture whose bits disagreed still compiled. They are derived now.
Three private members took the policy but used it for nothing.
apply_initial_operator_, build_evolve_result_ and propagate_one_ read P::picture straight
back out as the runtime value their callee already takes, duplicating a whole body per mode width
for no specialization. They take the runtime value.
FusedApply.h included picture/Picture.h. apply_fused_contract only tests a dependent
P::is_schrodinger, which needs no definition of P, so the include put the evaluation API and the
graph behind a header in the layer-build hot path. Dropped.
build_graph's seed vector was copied twice — once into the call, once inside it — though it is
dead at the call site. Moved.

Breaking changes (C++ only)

The Python API is untouched: schrodinger_cutoff keeps its historical spelling and the binder
resolves it into a PictureSpec at the boundary, and the schrodinger property still reads the same.

Declaration	Change
`MonomialPropagator` ctor	`std::optional<unsigned int> schrodinger_cutoff` → `const PictureSpec &picture`
`~MonomialPropagator`, `update_initial_operator`	no longer `virtual` (nothing overrode either)
`MPGraph` ctor	takes `ArrivalOrder`; the `(order, std::vector<Layer>)` overload is removed
`MPGraph::slice_graph`, `slice_view`, `growth`	removed — use `contraction_view()`, `clear()`, `replace_layer()`
`LayerGrowth`	renamed `ArrivalOrder`, enumerators `DescendingSlot`/`AscendingSlot`
`MPGraphView` ctor	`(std::span<const Layer>, bool)` instead of `(const std::vector<Layer> &, size_t, size_t, bool)`
`pare_graph`	drops `bool schrodinger`; takes no sweep direction at all
`detail::build_layer`	`bool schrodinger` → `Picture picture`
`detail::apply_fused_contract`	takes the picture policy as a template parameter, drops `bool schrodinger`
`detail::MPOperator::update_initial_operator`	`bool schrodinger` → `Picture picture`

Checklist

Tests added or updated to cover the changes
Documentation updated (docstrings, docs/, CONTRIBUTING.md) if needed
CHANGELOG / release notes updated if applicable

AI/LLM disclosure

I did not use LLM tooling, or used it only privately for ideation
I used the following tool to help write this PR description:
I used the following tool to generate or modify code: ClaudeCode:claude-opus-5

Important

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

Warning

If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.

The Heisenberg/Schrodinger choice was one `bool schrodinger_`, derived from whether `schrodinger_cutoff` held a value, and read at 17 decision points inside `MonomialPropagator`. The gate-order rule alone was written four times, and `contract_partially` carried two near-identical 14-line arms. Introduce a picture policy pair, built exactly like the existing `MajoranaAlgebra`/`PauliAlgebra` pair: `HeisenbergPicture` and `SchrodingerPicture` in `cpp/monoprop/picture/Picture.h`, each stating one picture's whole rule set (gate traversal direction, applied-angle sign, `map_params` phase, the live coefficient vector, the contraction partner), and one `with_picture()` bridge from the runtime value to the policy type. No inheritance, no virtual member, and still one propagator class -- the bindings, `PartitionGroup` and the dispatch generators keep their shape. Every public entry point binds the policy once with `with_picture()`. The whole private layer is then templated on the policy and never re-tests which picture it is in; there is deliberately no runtime-dispatching helper layer. The constructor now takes `PictureSpec = std::variant<Heisenberg, Schrodinger>` instead of `std::optional<unsigned int>`, so a Heisenberg run cannot carry a state cutoff and a Schrodinger run cannot omit one. That removes a dead `value_or` fallback which could never fire. Python keeps the historical "a cutoff selects Schrodinger" spelling; the variant is built at the binding boundary. `ContractSink` and `apply_fused_contract` take the policy as a template parameter, so the two branches that sat in the hot loop become `if constexpr`. The policy is bound at the sink only: templating `build_layer` itself would multiply the `with_algebra` scan above it into four instantiations per mode width instead of two. `bool schrodinger` becomes `Picture` in `MPGraph`, `pare_graph`, `build_layer` and `MPOperator::update_initial_operator`, so call sites no longer read `false` with a comment to explain it. Also drops the two vestigial `virtual` keywords -- nothing overrode either. Results are bit-identical to the previous implementation, verified by sha256 over the exact IEEE-754 bytes of 95 expectation-value and gradient fingerprints across both pictures, both algebras and both partition counts. BREAKING CHANGE: the `MonomialPropagator` C++ constructor takes a `PictureSpec` where it took `std::optional<unsigned int> schrodinger_cutoff`. The Python API is unchanged. Assisted-by: ClaudeCode:claude-opus-5

The Picture bullet still said cold sites reach a policy through `picture_*()` helpers. Those helpers were deleted when every public entry point started binding the policy once with `with_picture`, so the sentence described a layer that no longer exists. Record the rule that replaced it, and why `build_layer` binds at the sink rather than templating itself. Assisted-by: ClaudeCode:claude-opus-5

`MPGraph` held a `Picture` and asked `is_schrodinger()` in three places, so an exported class that only ever needed to know which end of its layer vector a new gate attaches to carried a simulation concept instead. Replace it with a graph-local `LayerGrowth { Back, Front }`. `core/Picture.h` leaves the exported header, `picture()` and `is_schrodinger()` go (they had no callers outside `MPGraph.cpp` and one `pare_graph` line), and the three ordering-sensitive members ask the question in one spelling, `grows_at_front()`. `layer_growth_of` in `picture/Picture.h` is the single translation point, in the direction that keeps the dependency one-way. `pare_graph` loses its `Picture` parameter outright: it already had the graph, so it now reads the sweep direction off `graph.growth()` and cannot be handed a direction that disagrees with the graph it is paring. Write down the invariant that made this bounded, because it is easy to assume the opposite. Layers are stored in DESCENDING optimizer-slot order under both growth ends, and that mapping is depended on outside the class -- MPFunctions' `prepare_evolved_operator` hard-codes `fill_mapped_params(..., reverse=true)`, its gradient loop hard-codes `count-1-i`, and `graph_gate_arrays_` hard-codes the same. All three are picture-free only because `append_layer` normalizes two arrival orders into one storage order. `slice_graph` is the documented exception: its result is ordered for replay, which coincides with that mapping only for `LayerGrowth::Back`. That invariant is also why the direction bit itself cannot be removed. Storing layers by absolute optimizer slot is impossible -- `propagate()` appends one gate at a time, and in the back-growth order each new gate takes slot 0 and shifts every existing layer up -- and storing in arrival order instead would push the picture into `ev`/`ev_and_grad`, which take none today. Results are bit-identical over the same 95 expectation-value and gradient fingerprints as the picture refactor. BREAKING CHANGE: `MPGraph`'s constructors take `LayerGrowth` where they took `Picture`, `picture()` and `is_schrodinger()` are gone, and `pare_graph` no longer takes a `Picture`. The Python API is unchanged. Assisted-by: ClaudeCode:claude-opus-5

Cleanup pass over the LayerGrowth change. The growth end is `gate_slot`'s slope, so it belongs on the picture policies beside it rather than in a second free function that re-branches on `Picture`. `HeisenbergPicture`/`SchrodingerPicture` now carry `layer_growth`, the `PicturePolicy` concept requires it, and `layer_growth_of` is gone -- which makes `picture/Picture.h`'s own "no runtime-dispatching helper layer on purpose" true again. The one caller binds it through `with_picture` like every other policy read. `MPGraph::grows_at_front()` becomes public so `pare_graph` asks the growth question in the same spelling as the class's own members instead of comparing enumerators in another translation unit. `slice_view`'s two returns collapse into one now that the reverse flag is visibly the same predicate, and `slice_graph`'s hand-rolled reverse-index loop becomes the reverse-iterator range it was spelling out. Trim the documentation to what the code cannot say: drop the paraphrase of three remote call sites in favour of naming them, drop the enum note that restated its own enumerator docs, and drop a comment that described `pare_graph`'s removed parameter. Correct two comments this refactor falsified -- `core/Picture.h` named two exported headers that no longer carry a `Picture`, and `replay_view()` plus `graph_gate_arrays_` still framed storage as "build"/"simulation" order, which holds only for `LayerGrowth::Back`. Also add the `MPGraph.h` include that `picture/Picture.h` was getting transitively through `MPFunctions.h`. Still bit-identical over the same 95 fingerprints. Assisted-by: ClaudeCode:claude-opus-5

contract_partially() is the only production caller of slice_graph()/slice_view(), and it always passes key == layers(): graph_gate_arrays_() sizes its arrays from graph_.layers(). The two functions therefore mean one view and one drain, so name them that way. contraction_view() needs no arms. active_end_index() - layers() equals active_begin_index(), so the window is the same under either growth end and only the reverse flag differs -- the key parameter was all that ever separated them. The inplace path also stops copying every Layer. It copied them out into an owned graph so the source could be drained before the evolution; with the order settled it can evolve over the graph's own layers and drain afterwards. slice_graph()/slice_view() stay for now, with their tests, so nothing loses coverage in this commit. Assisted-by: ClaudeCode:claude-opus-5

front_offset_ is always 0 in production. slice_graph()'s only caller passed key == layers(), so both arms emptied the store and maybe_compact_layers() only ever reached its clear() branch. The 4096 threshold, the half-size test and the erase arm were reachable from mp_graph_tests.cpp alone, which is why their tests go with them. With the dead prefix gone, layers() is layers_.size(), the layer offset is the layer index, and both views base at 0. contraction_view() becomes a one-liner in the header. This also discharges the slice-metadata problem: slice_graph() stamped its source's growth onto a slice whose layer order contradicted it under LayerGrowth::Front, and a prose warning was the only guard. pare_graph is now the sole caller of MPGraph(growth, layers), and it writes each layer back at the index it read it from. BREAKING CHANGE: MPGraph::slice_graph() and MPGraph::slice_view() are removed. Use contraction_view() for the replay and clear() for the drain. Assisted-by: ClaudeCode:claude-opus-5

The (container pointer, base, count) trio was reimplementing std::span. With the dead front prefix gone the window is always the whole store, so base_ has nothing left to carry and the caller folds the window into the span. sizeof(MPGraphView) drops from 40 to 24 bytes and the type becomes trivially copyable; it is copied by value into an EvalRequest on every evaluation. More importantly the class stops naming std::vector, so a later change to how MPGraph addresses its store does not have to reach into the view. Also records why this is not a std::ranges adaptor: the consumers index it rather than iterate it (it has no begin()/end()), one type has to serve both directions and std::ranges::any_view is C++26, and the type sits in exported signatures where a ranges adaptor would mangle library-version-dependently. BREAKING CHANGE: MPGraphView's constructor takes (std::span<const Layer>, bool) instead of (const std::vector<Layer> &, size_t, size_t, bool). Assisted-by: ClaudeCode:claude-opus-5

Four sites open-coded `n - 1 - x` to convert between store order and optimizer order, each deriving MPGraph's storage invariant from a prose comment. They now call slot_of_layer(), which is its own inverse, so one name serves both directions and two static_asserts pin it. Two other sites share the arithmetic but not the meaning, and keep their own names: Picture::gate_slot maps a simulation step to a slot (it coincides only under Heisenberg, where the sibling policy returns `i`), and MPGraphView::checked_layer_offset reverses a window. Also corrects fill_mapped_params' comment, which said the graph is traversed in simulation order. That holds for contract_partially's view, not for the evaluation path's, which is always the stored order. Assisted-by: ClaudeCode:claude-opus-5

pare_graph read graph.grows_at_front() to choose where its reachability sweep starts. That is not a layer-order question. The sweep must start at the end of the replay where the paring seed sits: Heisenberg seeds from the state, which contracts against the replay's last layer, and Schrodinger seeds from the operator, which enters at the first. The growth bit answered it only because each picture's build direction and its seed vector happen to agree. It now takes a PareSweep, supplied by the policy as `pare_sweep` beside `pare_seed`, which is what it describes. The old comment there was also wrong for LayerGrowth::Back: it claimed new layers attach where the circuit's latest operations are, but under back growth the last-appended layer is the circuit's earliest gate. grows_at_front() has no caller outside MPGraph again, so it goes back to private. BREAKING CHANGE: pare_graph() takes a trailing PareSweep argument. Assisted-by: ClaudeCode:claude-opus-5

layers_ grew at either end so that layer indices could stay in descending optimizer-slot order under both build directions. An ascending-slot build therefore inserted at layers_.begin() once per gate, which is O(n) per append. The store now only ever push_backs, and get_layer() maps a layer index onto arrival order instead. The layer order is unchanged, so nothing downstream moves; measured on this machine, appending 80k layers goes from 2863 ms to 15 ms (20k/40k/80k: 177/716/2863 ms before, 4.6/7.5/14.7 ms after -- quadratic to linear). Two consequences make the class simpler rather than cleverer: - contraction_view() is never reversed now, under either build direction: arrival order IS build order. replay_view() carries the flag instead, since the evaluation order is the layer order. - MPGraph(order, std::vector<Layer>) is gone. pare_graph copies the graph and calls the new replace_layer() instead of default-building a vector of n Layers, each of which allocated a LayerCore the sweep immediately overwrote. One constructor also means no object can carry metadata that contradicts its own layout. LayerGrowth becomes ArrivalOrder{DescendingSlot, AscendingSlot} and the policy member becomes arrival_order, which say what gate_slot's slope means rather than describing a store layout that no longer exists. Nothing reads the bit back, so the accessor is gone too: it enters at construction and stays inside the class. BREAKING CHANGE: LayerGrowth is renamed to ArrivalOrder, with its enumerators renamed to DescendingSlot/AscendingSlot; MPGraph::growth() and the MPGraph(ArrivalOrder, std::vector<Layer>) constructor are removed. Assisted-by: ClaudeCode:claude-opus-5

github-actions · 2026-08-18T11:26:15Z

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

Cleanup over the picture-policy and MPGraph work, from a review for reuse, simplification, efficiency and altitude. No functional change: all 95 bit-identity fingerprints are unchanged. Index mapping - slot_of_layer, and a new bounds-checked checked_layer_offset, move down into MPGraphViews.h so a graph and the views over it share one spelling of the store<->layer map and one out-of-range diagnostic. Both had a copy, with the same format string, and only MPGraph's went through slot_of_layer -- which its own comment claimed was the single spelling. - HeisenbergPicture::gate_slot calls slot_of_layer instead of open-coding n - 1 - i. Derivable policy bits - is_schrodinger and contract_reverse are derived, not hand-set. The PicturePolicy concept shape-checks exactly these constants and none of the behavioural members, so a picture whose bits disagreed still compiled. - contract_phase is gone: it was apply_sign under a second name. Both are the sign an applied angle carries, and contract_partially replays the very angles the build applied, so they cannot differ. - pare_sweep and PareSweep are gone; MPGraph::layer_of_unbuild_step answers it instead. The sweep starts at the last gate the build applied under either picture -- Heisenberg reaches it via layer n-1 (slot 0, its last step), Schrodinger via layer 0 (slot n-1, also its last step) -- because the seed is the result of the whole evolution, and reachability runs backwards from a result through the circuit that produced it. The removed comment called that agreement a coincidence; it is a derivation, and it was the parameter's only reason to exist. Wasted work - apply_initial_operator_, build_evolve_result_ and propagate_one_ used their policy parameter for one thing: reading P::picture back out as the runtime value their callee already takes. Each duplicated a whole body per mode width for no specialization. - build_graph's seed vector is moved into evolve_mode_graph_with_coeffs_ rather than copied; it is dead at the call site. - FusedApply.h no longer includes picture/Picture.h. apply_fused_contract only tests a dependent P::is_schrodinger, which needs no definition of P, so the include put the evaluation API and the graph behind a hot-path header. Also drops the dead picture() accessor, includes orphaned by the above, and the stale slice_graph/front_offset references the earlier removal left behind in the test README and the graph harness. Assisted-by: ClaudeCode:claude-opus-5

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>

robertodr added 10 commits August 18, 2026 13:07

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

robertodr commented Aug 18, 2026

View reviewed changes

Comment thread cpp/include/monoprop/MPGraph.h Outdated

robertodr commented Aug 18, 2026

View reviewed changes

Comment thread cpp/monoprop/detail/graph/MPGraphViews.h

robertodr commented Aug 18, 2026

View reviewed changes

Comment thread cpp/include/monoprop/MPFunctions.h Outdated

robertodr commented Aug 18, 2026

View reviewed changes

Comment thread cpp/monoprop/core/Picture.h Outdated

robertodr and others added 2 commits August 18, 2026 14:25

Apply suggestions from code review

2a636ab

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>

chore: run clang-format

78f7e36

Navigation Menu

Uh oh!

refactor(c++)!: 🎨 de-tangle picture decisions - #242

refactor(c++)!: 🎨 de-tangle picture decisions#242
robertodr wants to merge 13 commits into
mainfrom
refactor-compile-time-picture

robertodr commented Aug 18, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

robertodr commented Aug 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes (C++ only)

Checklist

AI/LLM disclosure

Uh oh!

github-actions Bot commented Aug 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robertodr commented Aug 18, 2026 •

edited

Loading