From 3a6ac7c7a30a1f0fb5d2683c8aa13396c211fe6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 11:03:50 +0000 Subject: [PATCH 01/14] test(propagator): :white_check_mark: record every mutator's effect on a live functional 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 --- AGENTS.md | 10 + cpp/include/monoprop/MonomialPropagator.h | 8 + cpp/tests/functional_validity.cpp | 335 ++++++++++++++++++++++ docs/content/docs/testing.mdx | 12 + tests/test_parameter_validation.py | 281 +++++++++++++++++- 5 files changed, 645 insertions(+), 1 deletion(-) create mode 100644 cpp/tests/functional_validity.cpp diff --git a/AGENTS.md b/AGENTS.md index 1fa7a18f..7842a6b9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -133,6 +133,13 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) ### Testing Structure +- `cpp/tests/functional_validity.cpp` is the **mutation table**: one row per public mutating method of + `MonomialPropagator`, recording what a functional built *before* that mutator ran does when called + *after* it — throw, or answer from its own snapshot. `MonomialPropagator::num_mutating_methods` pins + the roster and the table `static_assert`s against it, so adding a mutator means bumping that + constant and adding a row (the build fails until you do). + `tests/test_parameter_validation.py::TestFunctionalValidityTable` mirrors the same rows through the + Python front end, over `monoprop_PARTITIONS=off` and `=auto`. - `tests/cases.py`: Parametrized test cases using `pytest-cases`; `load_problem()` loads a `tests/data/*.msgpack` fixture directly into the public API (`MonomialCircuit` + `MonomialOperator`). C++ tests use the equivalent `test_utils::load_case()` in `cpp/tests/TestData.h` - Fixture msgpack schema is documented in `tests/data/README.md` - Tests validate against exact solutions for small systems @@ -161,6 +168,9 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) 7. Add Python bindings in `src/monoprop/bindings/binder.h` 8. Regenerate bindings with `tools/generate-binders.py` 9. Test with both C++ and Python tests +10. If the new method mutates a `MonomialPropagator`, bump + `MonomialPropagator::num_mutating_methods` and add its row to the mutation table (see "Testing + Structure"): a mutator with no row leaves its effect on a live functional unrecorded. ## Documentation Maintenance Policy diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index f80f3e87..1a4aedc2 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -94,6 +94,14 @@ class MonomialPropagator { static constexpr auto num_modes{NumModes}; static constexpr auto storage_num_modes{NumModes}; + /// How many public methods mutate this propagator, and so may invalidate a live functional. + // cpp/tests/functional_validity.cpp static_asserts that its table has a row for each, so bumping + // this when adding a mutator breaks that build until the new method's effect on a functional is + // recorded. The roster: build_graph, propagate, contract_partially, update_initial_operator, + // set_parameter_mapping, update_cutoff, update_cutoff_type, update_basis_change, + // update_lower_atol, update_upper_atol. + static constexpr size_t num_mutating_methods{10}; + auto logical_num_modes() const -> size_t { return logical_num_modes_; } /// Term count on this rank (allreduce for global). diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp new file mode 100644 index 00000000..8ea61cd7 --- /dev/null +++ b/cpp/tests/functional_validity.cpp @@ -0,0 +1,335 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// What each public mutator does to a functional that was built before it ran. The table is the +// contract: one row per mutating method, asserted for the value and the gradient functional, with +// and without a pare threshold, in both pictures. A cell that says Answers and carries a defect note +// records behaviour that is wrong today and is fixed by the stage the note names. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "monoprop/MonomialPropagator.h" +#include "monoprop/detail/mpi/MPICompat.h" + +using namespace monoprop; +namespace tt = boost::test_tools; + +namespace { + +constexpr size_t kNumModes = 2; +using Prop = MonomialPropagator; + +// Small enough to reason about by hand, large enough that both gates rotate something. +constexpr double kPareThreshold = 1e-12; +const VecD kBaseParams{0.3, 0.7}; +// One gate per Hamiltonian term, and the terms carry different weights, so the answer is not +// symmetric under swapping the two angles -- which is what makes the set_parameter_mapping row bite. +const std::vector kBaseGates{VecZ{0}, VecZ{2}}; + +auto make_propagator(bool schrodinger) -> Prop { + OperatorDict initial_ham; + initial_ham[VecZ{0, 1}] = std::complex{0.0, 1.0}; + initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; + const auto cutoff = static_cast(2 * kNumModes); + return Prop(initial_ham, + cutoff, + VecZ{0, 1}, + schrodinger ? std::optional{cutoff} : std::nullopt, + MPI_COMM_SELF, + std::nullopt, + std::nullopt, + CutoffType::Support, + std::nullopt); +} + +auto build_base_graph(Prop &prop) -> void { + prop.build_graph(kBaseGates, VecZ{0, 1}, VecD{1.0, 1.0}); +} + +// The mutators, one per public mutating method. Each runs against a propagator that already carries +// the two-layer base graph, except propagate(), which refuses a non-empty graph (see +// propagate_on_non_empty_graph_leaves_functional_valid). + +auto mutate_build_graph(Prop &prop) -> void { + prop.build_graph({VecZ{1}}, VecZ{0}, VecD{1.0}); +} + +auto mutate_propagate(Prop &prop) -> void { + prop.propagate({VecZ{0}}, VecZ{0}, VecD{1.0}, VecD{0.4}); +} + +auto mutate_contract_partially(Prop &prop) -> void { + prop.contract_partially(kBaseParams, /*inplace=*/true); +} + +auto mutate_update_initial_operator(Prop &prop) -> void { + OperatorDict updated; + updated[VecZ{0, 1}] = std::complex{0.0, 2.75}; + updated[VecZ{2, 3}] = std::complex{0.0, 0.5}; + prop.update_initial_operator(updated); +} + +// Swaps which parameter drives which layer, so the answer moves whenever the two angles differ. +auto mutate_set_parameter_mapping(Prop &prop) -> void { + prop.set_parameter_mapping(VecZ{1, 0}); +} + +auto mutate_update_cutoff(Prop &prop) -> void { + prop.update_cutoff(2); +} + +auto mutate_update_cutoff_type(Prop &prop) -> void { + prop.update_cutoff_type(CutoffType::Length); +} + +auto mutate_update_basis_change(Prop &prop) -> void { + prop.update_basis_change(std::vector{VecZ{0}, VecZ{1}, VecZ{0, 1, 2}, VecZ{0, 1, 3}}); +} + +auto mutate_update_lower_atol(Prop &prop) -> void { + prop.update_lower_atol(1e-12); +} + +auto mutate_update_upper_atol(Prop &prop) -> void { + prop.update_upper_atol(1e-3); +} + +// What calling the pre-built functional does after the row's mutator ran. +enum class Outcome : std::uint8_t { + Stale, // throws, reporting the propagator moved under the functional + Answers, // returns, and returns exactly what it returned before the mutation +}; + +struct MutatorRow { + std::string_view method; // the public method this row covers + void (*apply)(Prop &); // + bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted + Outcome exact; // pare_threshold == nullopt + Outcome pared; // pare_threshold == kPareThreshold + std::string_view rationale; // +}; + +constexpr std::array kMutatorTable{ + MutatorRow{.method = "build_graph", + .apply = &mutate_build_graph, + .needs_empty_graph = false, + .exact = Outcome::Stale, + .pared = Outcome::Answers, + .rationale = "DEFECT (fixed in stage 2): the pared plan owns its layers, so its layer " + "count cannot move and the live-graph check never fires."}, + MutatorRow{.method = "propagate", + .apply = &mutate_propagate, + .needs_empty_graph = true, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "DEFECT (fixed in stage 2): propagate() leaves the layer count at zero and " + "does not touch the epoch, so neither check sees the re-evolved operator."}, + MutatorRow{.method = "contract_partially", + .apply = &mutate_contract_partially, + .needs_empty_graph = false, + .exact = Outcome::Stale, + .pared = Outcome::Answers, + .rationale = "DEFECT (fixed in stage 2): as build_graph, the pared plan's layer count is fixed."}, + MutatorRow{.method = "update_initial_operator", + .apply = &mutate_update_initial_operator, + .needs_empty_graph = false, + .exact = Outcome::Stale, + .pared = Outcome::Stale, + .rationale = "The epoch check fires. Stage 4 turns this into a weight refresh, except for " + "a pared Schrodinger plan."}, + MutatorRow{.method = "set_parameter_mapping", + .apply = &mutate_set_parameter_mapping, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "DEFECT (fixed in stage 2): relabelling is in place, so the layer count and " + "the epoch both hold and the plan keeps replaying the old labels."}, + MutatorRow{.method = "update_cutoff", + .apply = &mutate_update_cutoff, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "Intended: a cutoff gates the next build and changes nothing the plan holds."}, + MutatorRow{.method = "update_cutoff_type", + .apply = &mutate_update_cutoff_type, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "Intended: as update_cutoff."}, + MutatorRow{.method = "update_basis_change", + .apply = &mutate_update_basis_change, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "Intended: as update_cutoff."}, + MutatorRow{.method = "update_lower_atol", + .apply = &mutate_update_lower_atol, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "Intended: as update_cutoff."}, + MutatorRow{.method = "update_upper_atol", + .apply = &mutate_update_upper_atol, + .needs_empty_graph = false, + .exact = Outcome::Answers, + .pared = Outcome::Answers, + .rationale = "Intended: as update_cutoff."}, +}; + +// A mutating method with no row would leave its effect on a live functional unrecorded, so the count +// is pinned to the roster on the class. Adding a mutator means bumping that constant, which breaks +// this build until a row lands here. +constexpr auto distinct_methods() -> size_t { + size_t distinct = 0; + for (size_t i = 0; i < kMutatorTable.size(); ++i) { + bool seen_earlier = false; + for (size_t j = 0; j < i; ++j) { + seen_earlier = seen_earlier || kMutatorTable[j].method == kMutatorTable[i].method; + } + distinct += seen_earlier ? 0U : 1U; + } + return distinct; +} + +static_assert(distinct_methods() == Prop::num_mutating_methods, + "cpp/tests/functional_validity.cpp must carry one row per public mutating method of " + "MonomialPropagator; see num_mutating_methods in MonomialPropagator.h."); + +// Both functional kinds behind one signature; the gradient kind is judged on its value component. +auto make_call(Prop &prop, bool gradient, std::optional pare_threshold) -> std::function { + if (gradient) { + auto fn = prop.expectation_value_and_gradient_functional(pare_threshold); + return [fn = std::move(fn)](const VecD ¶ms) { return fn(params).first; }; + } + auto fn = prop.expectation_value_functional(pare_threshold); + return [fn = std::move(fn)](const VecD ¶ms) { return fn(params); }; +} + +auto run_row(const MutatorRow &row, bool schrodinger, bool gradient, std::optional pare_threshold) -> void { + auto prop = make_propagator(schrodinger); + if (!row.needs_empty_graph) { + build_base_graph(prop); + } + const VecD params = row.needs_empty_graph ? VecD{} : kBaseParams; + + auto call = make_call(prop, gradient, pare_threshold); + const double before = call(params); + + row.apply(prop); + + const auto expected = pare_threshold.has_value() ? row.pared : row.exact; + if (expected == Outcome::Stale) { + BOOST_CHECK_EXCEPTION(call(params), std::runtime_error, [](const std::runtime_error &e) { + BOOST_TEST_INFO("message: " << e.what()); + return std::string_view(e.what()).find("MP object has been modified") != std::string_view::npos; + }); + return; + } + // Answers: the plan replays its own snapshot, so the number cannot have moved. + double after = 0.0; + BOOST_REQUIRE_NO_THROW(after = call(params)); + BOOST_TEST(after == before, tt::tolerance(1e-12)); +} + +auto run_table(bool schrodinger, std::optional pare_threshold) -> void { + for (const auto &row : kMutatorTable) { + for (const bool gradient : {false, true}) { + BOOST_TEST_CONTEXT("method=" << row.method << " gradient=" << gradient << " rationale=" << row.rationale) { + run_row(row, schrodinger, gradient, pare_threshold); + } + } + } +} + +} // namespace + +BOOST_AUTO_TEST_CASE(functional_validity_table_heisenberg_exact) { + run_table(/*schrodinger=*/false, std::nullopt); +} + +BOOST_AUTO_TEST_CASE(functional_validity_table_heisenberg_pared) { + run_table(/*schrodinger=*/false, kPareThreshold); +} + +BOOST_AUTO_TEST_CASE(functional_validity_table_schrodinger_exact) { + run_table(/*schrodinger=*/true, std::nullopt); +} + +BOOST_AUTO_TEST_CASE(functional_validity_table_schrodinger_pared) { + run_table(/*schrodinger=*/true, kPareThreshold); +} + +// contract_partially(inplace=false) mutates nothing, so it belongs outside the table: it must never +// invalidate a functional. +BOOST_AUTO_TEST_CASE(contract_partially_out_of_place_keeps_functional_valid) { + auto prop = make_propagator(/*schrodinger=*/false); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + const double before = call(kBaseParams); + + prop.contract_partially(kBaseParams, /*inplace=*/false); + + BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); +} + +// propagate() refuses to run on top of a stored graph, so it cannot invalidate a functional built +// against one: the rejection leaves the propagator untouched. +BOOST_AUTO_TEST_CASE(propagate_on_non_empty_graph_leaves_functional_valid) { + auto prop = make_propagator(/*schrodinger=*/false); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + const double before = call(kBaseParams); + + BOOST_CHECK_THROW(mutate_propagate(prop), std::runtime_error); + + BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); +} + +// The two Answers-with-a-defect rows above only say the number did not move. These two say why that +// is wrong: the propagator's own answer *did* move, so the functional is now reporting a circuit +// nobody asked about. Stage 2 turns both calls into throws. + +BOOST_AUTO_TEST_CASE(set_parameter_mapping_silently_desynchronises_functional) { + auto prop = make_propagator(/*schrodinger=*/false); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + const double before = call(kBaseParams); + + mutate_set_parameter_mapping(prop); + + BOOST_TEST(prop.expectation_value(kBaseParams) != before); + BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); +} + +BOOST_AUTO_TEST_CASE(pared_functional_silently_survives_build_graph) { + auto prop = make_propagator(/*schrodinger=*/false); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, kPareThreshold); + const double before = call(kBaseParams); + + mutate_build_graph(prop); + + BOOST_TEST(prop.expectation_value(kBaseParams) != before); + BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); +} diff --git a/docs/content/docs/testing.mdx b/docs/content/docs/testing.mdx index 8390991b..f0c573c8 100644 --- a/docs/content/docs/testing.mdx +++ b/docs/content/docs/testing.mdx @@ -188,6 +188,18 @@ BOOST_AUTO_TEST_CASE(my_basic_check) { } ``` +#### The mutation table + +`cpp/tests/functional_validity.cpp` records, for every public method that mutates a +`MonomialPropagator`, what a functional built before that call does when called after it: throw +("stale"), or answer from its own snapshot ("answers"). Each row is asserted for the value and the +gradient functional, with and without a pare threshold, in both pictures. + +The roster is pinned by `MonomialPropagator::num_mutating_methods`, and the table `static_assert`s +its row count against it — so adding a mutating method means bumping that constant, which breaks the +build until the new method gets a row. `tests/test_parameter_validation.py::TestFunctionalValidityTable` +mirrors the same rows through the Python front end, over both `monoprop_PARTITIONS=off` and `=auto`. + #### Data-driven test using reference fixtures Use the `ExampleDataFix` fixture class from `TestUtilities.h` to load the same msgpack data as Python tests, and `BOOST_DATA_TEST_CASE_F` to parametrize over it: diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index a38bbbea..ca18c4c7 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -18,7 +18,13 @@ import pytest -from monoprop import Circuit, ExpGate, MajoranaPropagator, PauliPropagator +from monoprop import ( + Circuit, + ExpGate, + MajoranaPropagator, + PauliPropagator, + jordan_wigner_basis_change, +) from monoprop.majorana import MajoranaOperator from monoprop.pauli import PauliOperator @@ -200,6 +206,279 @@ def test_functional_invalidated_after_initial_operator_update( assert rebuilt_expval == pytest.approx(mp.expval(parameters)) +class TestFunctionalValidityTable: + """What each public mutator does to a functional built before it ran. + + The Python mirror of ``cpp/tests/functional_validity.cpp``: same rows, same expectations, run + over both partition settings. The build-time coverage gate lives on the C++ side + (``MonomialPropagator::num_mutating_methods``); here the roster is asserted as data. + """ + + _MODES = 2 + _CUTOFF = 4 + _PARAMS = (0.3, 0.7) + _PARE_THRESHOLD = 1e-12 + + # One gate per Hamiltonian term, and the terms carry different weights, so the answer is not + # symmetric under swapping the two angles -- which is what makes the parameter_mapping row bite. + @staticmethod + def _generator(index): + return MajoranaOperator({(index,): 1.0}, num_modes=2) + + @classmethod + def _base_circuit(cls): + return Circuit( + (ExpGate(cls._generator(0)), ExpGate(cls._generator(2))), + cls._MODES, + cls._PARAMS, + ) + + @classmethod + def _propagator(cls, comm, *, schrodinger, with_graph): + mp = MajoranaPropagator( + MajoranaOperator({(0, 1): 1.0j, (2, 3): 0.5j}, num_modes=cls._MODES), + [0, 1], + cutoff=cls._CUTOFF, + schrodinger_cutoff=cls._CUTOFF if schrodinger else None, + comm=comm, + ) + if with_graph: + mp.build_graph(cls._base_circuit()) + return mp + + # The mutators, one per public mutating method. + @classmethod + def _mutate_build_graph(cls, mp): + mp.build_graph(Circuit((ExpGate(cls._generator(1)),), cls._MODES, (0.4,))) + + @classmethod + def _mutate_propagate(cls, mp): + mp.propagate(Circuit((ExpGate(cls._generator(0)),), cls._MODES, (0.4,))) + + @classmethod + def _mutate_contract_partially(cls, mp): + mp.contract_partially(list(cls._PARAMS), inplace=True) + + @classmethod + def _mutate_update_initial_operator(cls, mp): + mp.update_initial_operator( + MajoranaOperator({(0, 1): 2.75j, (2, 3): 0.5j}, num_modes=cls._MODES) + ) + + @staticmethod + def _mutate_parameter_mapping(mp): + mp.parameter_mapping = [1, 0] + + @staticmethod + def _mutate_cutoff(mp): + mp.cutoff = 2 + + @staticmethod + def _mutate_cutoff_type(mp): + mp.cutoff_type = "length" + + @classmethod + def _mutate_basis_change(cls, mp): + # No front-end setter; the engine property is the only way in (see tests/test_basis.py). + mp._simulator.basis_change = jordan_wigner_basis_change(cls._MODES) + + @staticmethod + def _mutate_lower_atol(mp): + mp.lower_atol = 1e-12 + + @staticmethod + def _mutate_upper_atol(mp): + mp.upper_atol = 1e-3 + + # (method, mutator, needs_empty_graph, exact, pared, rationale). "stale" = the call must throw, + # "answers" = it must return exactly what it returned before the mutation. + ROWS = ( + ( + "build_graph", + "_mutate_build_graph", + False, + "stale", + "answers", + "DEFECT (fixed in stage 2): the pared plan owns its layers, so its layer count cannot " + "move and the live-graph check never fires.", + ), + ( + "propagate", + "_mutate_propagate", + True, + "answers", + "answers", + "DEFECT (fixed in stage 2): propagate() leaves the layer count at zero and does not " + "touch the epoch, so neither check sees the re-evolved operator.", + ), + ( + "contract_partially", + "_mutate_contract_partially", + False, + "stale", + "answers", + "DEFECT (fixed in stage 2): as build_graph, the pared plan's layer count is fixed.", + ), + ( + "update_initial_operator", + "_mutate_update_initial_operator", + False, + "stale", + "stale", + "The epoch check fires. Stage 4 turns this into a weight refresh, except for a pared " + "Schrodinger plan.", + ), + ( + "parameter_mapping", + "_mutate_parameter_mapping", + False, + "answers", + "answers", + "DEFECT (fixed in stage 2): relabelling is in place, so the layer count and the epoch " + "both hold and the plan keeps replaying the old labels.", + ), + ( + "cutoff", + "_mutate_cutoff", + False, + "answers", + "answers", + "Intended: a cutoff gates the next build and changes nothing the plan holds.", + ), + ( + "cutoff_type", + "_mutate_cutoff_type", + False, + "answers", + "answers", + "Intended: as cutoff.", + ), + ( + "basis_change", + "_mutate_basis_change", + False, + "answers", + "answers", + "Intended: as cutoff.", + ), + ( + "lower_atol", + "_mutate_lower_atol", + False, + "answers", + "answers", + "Intended: as cutoff.", + ), + ( + "upper_atol", + "_mutate_upper_atol", + False, + "answers", + "answers", + "Intended: as cutoff.", + ), + ) + + def test_table_covers_every_public_mutator(self): + assert {row[0] for row in self.ROWS} == { + "build_graph", + "propagate", + "contract_partially", + "update_initial_operator", + "parameter_mapping", + "cutoff", + "cutoff_type", + "basis_change", + "lower_atol", + "upper_atol", + } + + @pytest.mark.parametrize("row", ROWS, ids=[row[0] for row in ROWS]) + @pytest.mark.parametrize("partitions", ["off", "auto"]) + @pytest.mark.parametrize("pared", [False, True], ids=["exact", "pared"]) + @pytest.mark.parametrize( + "schrodinger", [False, True], ids=["heisenberg", "schrodinger"] + ) + @pytest.mark.parametrize( + "functional_name", + [ + "expectation_value_functional", + "expectation_value_and_gradient_functional", + ], + ) + def test_mutator_effect_on_live_functional( + self, + monkeypatch, + serial_comm, + functional_name, + schrodinger, + pared, + partitions, + row, + ): + method, mutator, needs_empty_graph, exact, pared_outcome, rationale = row + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + + mp = self._propagator( + serial_comm, schrodinger=schrodinger, with_graph=not needs_empty_graph + ) + parameters = [] if needs_empty_graph else list(self._PARAMS) + threshold = self._PARE_THRESHOLD if pared else None + functional = getattr(mp, functional_name)(threshold) + + def call(): + result = functional(parameters) + return result[0] if isinstance(result, tuple) else result + + before = call() + getattr(self, mutator)(mp) + + expected = pared_outcome if pared else exact + if expected == "stale": + with pytest.raises(RuntimeError, match=r"MP object has been modified"): + call() + else: + assert call() == pytest.approx(before), f"{method}: {rationale}" + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_parameter_mapping_silently_desynchronises_functional( + self, monkeypatch, serial_comm, partitions + ): + """Defect 2, made falsifiable: the relabel moves the propagator's own answer while the + functional built before it keeps returning the old one. Stage 2 makes the call throw. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) + functional = mp.expectation_value_functional() + parameters = list(self._PARAMS) + before = functional(parameters) + + self._mutate_parameter_mapping(mp) + + assert mp.expval(parameters) != pytest.approx(before) + assert functional(parameters) == pytest.approx(before) + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_pared_functional_silently_survives_build_graph( + self, monkeypatch, serial_comm, partitions + ): + """Defect 1, made falsifiable: appending a layer moves the propagator's own answer while a + pared functional, whose owned layer count cannot change, keeps returning the old one. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) + functional = mp.expectation_value_functional(self._PARE_THRESHOLD) + parameters = list(self._PARAMS) + before = functional(parameters) + + self._mutate_build_graph(mp) + + # The appended gate claims a fresh angle, so the propagator's own axis is now three long + # while the functional still answers for the two-gate circuit it was built against. + assert mp.expval([*parameters, 0.4]) != pytest.approx(before) + assert functional(parameters) == pytest.approx(before) + + class TestEvolvedOperatorBothPictures: def test_schrodinger_returns_state_dict(self, serial_comm): operator = MajoranaOperator({(0, 1): 1.0j}, num_modes=2) From 5ee0e9e914c66cdf7b9a19bfb59a3c01ecaeb0c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 11:28:11 +0000 Subject: [PATCH 02/14] refactor(propagator): :recycle: give the evaluation functionals a name 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: 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 and ExpectationValueAndGradientFunctional. 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 --- AGENTS.md | 8 + cpp/include/monoprop/CMakeLists.txt | 1 + cpp/include/monoprop/Functional.h | 184 ++++++++++++++++++ cpp/include/monoprop/MonomialPropagator.h | 21 +- .../MonomialPropagator.inl | 118 ++++------- cpp/tests/functional_validity.cpp | 12 ++ src/monoprop/bindings/binder.h | 26 +++ tests/test_parameter_validation.py | 16 ++ 8 files changed, 294 insertions(+), 92 deletions(-) create mode 100644 cpp/include/monoprop/Functional.h diff --git a/AGENTS.md b/AGENTS.md index 7842a6b9..1798d628 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,6 +71,14 @@ Key files: - `cpp/include/monoprop/MonomialPropagator.h`: the single templated C++ engine `MonomialPropagator` (the Majorana/Pauli choice is a runtime `Basis`, not a separate class). Its `only_rotate_len_k` arguments use `std::optional`; `std::nullopt` means no gate-application length cap. +- `cpp/include/monoprop/Functional.h`: the two functional objects, + `ExpectationValueFunctional` and `ExpectationValueAndGradientFunctional`. Each is + a handle on one shared `detail::FunctionalPlan` — 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 value and the + gradient functional over one snapshot share one plan. A functional borrows from its propagator (the + inverted index always, the graph unless pared), so it must not outlive it; the bindings pin that with + `nb::keep_alive<0, 1>`. - `src/monoprop/bindings/binder.h`: hand-written binding template; `tools/generate-*.py` generate the per-mode-width `bindings.cpp` and `_dispatch.py` from it (do not hand-edit the generated files). Both generators take the 32-mode storage-block rule from `tools/_binding_layout.py` — they must diff --git a/cpp/include/monoprop/CMakeLists.txt b/cpp/include/monoprop/CMakeLists.txt index 98159824..9fb002ea 100644 --- a/cpp/include/monoprop/CMakeLists.txt +++ b/cpp/include/monoprop/CMakeLists.txt @@ -16,6 +16,7 @@ target_sources( "${PROJECT_BINARY_DIR}/include/${PROJECT_NAME}/Info.h" "${PROJECT_BINARY_DIR}/include/${PROJECT_NAME}/Variants.h" "${PROJECT_SOURCE_DIR}/cpp/include/${PROJECT_NAME}/Evolution.h" + "${PROJECT_SOURCE_DIR}/cpp/include/${PROJECT_NAME}/Functional.h" "${PROJECT_SOURCE_DIR}/cpp/include/${PROJECT_NAME}/MPFunctions.h" "${PROJECT_SOURCE_DIR}/cpp/include/${PROJECT_NAME}/MPGraph.h" "${PROJECT_SOURCE_DIR}/cpp/include/${PROJECT_NAME}/MonomialPropagator.h" diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h new file mode 100644 index 00000000..bc71a20d --- /dev/null +++ b/cpp/include/monoprop/Functional.h @@ -0,0 +1,184 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include +#include +#include +#include +#include + +#include "monoprop/MPFunctions.h" +#include "monoprop/MPGraph.h" +#include "monoprop/TypeAliases.h" +#include "monoprop/Validation.h" +#include "monoprop/detail/evolution/CosineRecomputeCallbacks.h" +#include "monoprop/detail/mpi/Comm.h" +#include "monoprop/detail/partition/PartitionGroup.h" + +namespace monoprop { + +template +class MonomialPropagator; + +namespace detail { + +/// One propagator snapshot a functional replays, plus the checks that say the snapshot is still that +/// propagator's own. +/// +/// Immutable once built and held by `shared_ptr`, so the value and the gradient functional over +/// the same snapshot share one plan. Every field is either owned or, where the comment says so, +/// borrowed from the propagator — which is why a functional must not outlive it. +template +class FunctionalPlan { +public: + /// A single-partition propagator's snapshot: one replay of its graph against its operator. + struct Local { + double core_term{0.0}; ///< the identity term, added to the summed expectation value + // Owns its rows and snapshots the term count: the operator's sparse rows grow by push_back as + // terms are appended, so a view would both dangle and outrun `op`. + EvalState state; ///< the contraction partner, sparse (Heisenberg) or dense (Schrodinger) + VecD op; ///< un-evolved operator coefficients, copied out of the propagator + VecZ parameter_mapping; ///< optimizer order: which parameter drives graph layer i + VecD gen_coeffs; ///< optimizer order, parallel to parameter_mapping + // One owning handle either way: pare hands back a heap-owned MPGraph the plan must keep alive + // (`cos` holds pointers into its layers' stored cos); non-pare aliases the propagator's graph_ + // through a shared_ptr with an empty owner block, so it stays live only while the propagator does. + std::shared_ptr graph; + // The folds keep raw column pointers into the propagator's inverted index, so this plan must not + // outlive the propagator either. + CosCallbacks cos; + mpi::Comm comm{}; ///< real MPI across nodes, or the in-process comm across partitions + + // Validity, checked before every replay. `initial_operator_epoch` is aliased rather than copied + // because the check needs the propagator's live counter, as `graph->layers()` needs its live graph. + const size_t *initial_operator_epoch{nullptr}; + size_t expected_initial_operator_epoch{0}; + size_t expected_graph_layers{0}; + }; + + /// A partition facade's snapshot: one child plan per partition, replayed together. + struct Fanout { + // Borrowed, like every other field: the group belongs to the facade propagator. + partition::PartitionGroup *group{nullptr}; + std::vector> partitions; ///< in partition order + }; + + FunctionalPlan(size_t num_params, Local local) : num_params_(num_params), shape_(std::move(local)) {} + + FunctionalPlan(size_t num_params, Fanout fanout) : num_params_(num_params), shape_(std::move(fanout)) {} + + /// The parameter-axis length the plan was built against; a call must supply exactly this many. + auto num_params() const -> size_t { return num_params_; } + + /// Throw unless `params` fits and the propagator still holds what the plan replays. + // A facade validates nothing here: the state each check reads lives on the partitions, so each child + // plan checks itself on its own master thread, inside evaluate(). + auto validate(const VecD ¶ms) const -> void { + const auto *local = std::get_if(&shape_); + if (local == nullptr) { + return; + } + validate_expected_initial_operator(*local->initial_operator_epoch, local->expected_initial_operator_epoch); + validate_functional_call(params, num_params_); + validate_expected_graph_layers(local->graph->layers(), local->expected_graph_layers); + } + + /// Replay the snapshot: `fn(request, comm, cos)` locally, or partition 0's answer on a facade. + template > + auto evaluate(Fn &&fn, const VecD ¶ms) const -> R { + if (const auto *fanout = std::get_if(&shape_)) { + // Each partition allreduces internally, so partition 0 already carries the global answer. + // The fan-out must reach every master: the partitions' collectives are barrier-synced. + return partition::collect_on_all(*fanout->group, [&](int r) -> R { + return fanout->partitions[static_cast(r)]->evaluate(fn, params); + })[0]; + } + validate(params); + const auto &local = std::get(shape_); + return fn(EvalRequest{.e_core = local.core_term, + .state = local.state, + .op = local.op, + .parameter_mapping = local.parameter_mapping, + .gen_coeffs = local.gen_coeffs, + .graph = local.graph->replay_view(), + .params = params}, + local.comm, + local.cos); + } + +private: + size_t num_params_{0}; + std::variant shape_; +}; + +} // namespace detail + +/// A reusable expectation value over one propagator snapshot: `fn(parameters) -> double`. +/// +/// Built by MonomialPropagator::expectation_value_functional(). It borrows from the propagator that +/// made it (see detail::FunctionalPlan), so it must not outlive it, and a structural change to the +/// propagator makes a call throw rather than answer. +template +class ExpectationValueFunctional { +public: + /// The parameter-axis length this functional was built against. + auto num_params() const -> size_t { return plan_->num_params(); } + + auto operator()(const VecD ¶meters) const -> double { + return plan_->evaluate( + [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> double { + return ev(request, comm, cos); + }, + parameters); + } + +private: + friend class MonomialPropagator; + + explicit ExpectationValueFunctional(std::shared_ptr> plan) + : plan_(std::move(plan)) {} + + std::shared_ptr> plan_; +}; + +/// As ExpectationValueFunctional, plus the gradient from the same backward pass: +/// `fn(parameters) -> (value, gradient)`, the gradient in parameter-axis order. +template +class ExpectationValueAndGradientFunctional { +public: + /// The parameter-axis length this functional was built against. + auto num_params() const -> size_t { return plan_->num_params(); } + + auto operator()(const VecD ¶meters) const -> std::pair { + return plan_->evaluate( + [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> std::pair { + return ev_and_grad(request, comm, cos); + }, + parameters); + } + +private: + friend class MonomialPropagator; + + explicit ExpectationValueAndGradientFunctional(std::shared_ptr> plan) + : plan_(std::move(plan)) {} + + std::shared_ptr> plan_; +}; + +} // namespace monoprop diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index 1a4aedc2..0eb35451 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -34,6 +34,7 @@ #include #include "monoprop/Evolution.h" +#include "monoprop/Functional.h" #include "monoprop/MPFunctions.h" #include "monoprop/MPGraph.h" #include "monoprop/TypeAliases.h" @@ -263,11 +264,12 @@ class MonomialPropagator { auto expectation_value_and_gradient(const VecD ¶meters) -> std::pair; /// `pare_threshold` is the edge-retention cutoff for a masked plan; nullopt keeps the exact graph. + /// The result borrows from this propagator, so it must not outlive it. auto expectation_value_functional(std::optional pare_threshold = std::nullopt) - -> std::function; + -> ExpectationValueFunctional; auto expectation_value_and_gradient_functional(std::optional pare_threshold = std::nullopt) - -> std::function(const VecD &)>; + -> ExpectationValueAndGradientFunctional; /// Contract the graph into the operator (Heisenberg) or state (Schrodinger). `inplace` consumes the /// graph and updates internal state; otherwise nothing is mutated. Core term excluded either way. @@ -285,15 +287,6 @@ class MonomialPropagator { virtual auto update_initial_operator(const OperatorDict &op_dict) -> void { apply_initial_operator_(op_dict); } protected: - static inline const auto ev_fn = [](const EvalRequest &request, - mpi::Comm comm, - const detail::CosCallbacks &cos) -> double { return ev(request, comm, cos); }; - - static inline const auto ev_and_grad_fn = - [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> std::pair { - return ev_and_grad(request, comm, cos); - }; - /// Distribute op_dict across ranks and apply this rank's share; returns its new (terms, coeffs) /// so caches can refresh. auto apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD>; @@ -462,9 +455,9 @@ class MonomialPropagator { VecD *fused_scale_coeffs = nullptr, bool *fused_scale = nullptr) -> std::shared_ptr; - template > - auto make_functional_(Fn &&func, std::optional pare_threshold) -> std::function; + // One plan serves both functional kinds: it holds the snapshot and the validity checks, not the + // choice of what to compute. On a facade it holds one child plan per partition. + auto make_plan_(std::optional pare_threshold) -> std::shared_ptr>; // Reconstruct the optimizer-order (parameter_mapping, gen_coeffs) arrays from the layers' gate info. auto graph_gate_arrays_() const -> std::pair; diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 117590ec..442f689f 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -916,40 +916,48 @@ auto build_cos_callbacks(const detail::InvertedIndex &inverted_index, } template -template -auto MonomialPropagator::make_functional_(Fn &&func, std::optional pare_threshold) - -> std::function { +auto MonomialPropagator::make_plan_(std::optional pare_threshold) + -> std::shared_ptr> { + using Plan = detail::FunctionalPlan; + + if (partition_group_) { + // One fan-out for both functional kinds: the plan holds the snapshot, not the choice of what to + // compute. The children are built on the partitions' own masters, where their state lives. + typename Plan::Fanout fanout; + fanout.group = partition_group_.get(); + fanout.partitions = map_partitions_([&](MonomialPropagator &s) { return s.make_plan_(pare_threshold); }); + // graph_gate_arrays_() reads partition 0: the graph structure and gate info are identical on + // every partition. A facade validates nothing itself (see FunctionalPlan::validate). + return std::make_shared(expected_num_params(graph_gate_arrays_().first), std::move(fanout)); + } + + typename Plan::Local local; + auto gate_arrays = graph_gate_arrays_(); - auto parameter_mapping = std::move(gate_arrays.first); - auto gen_coeffs = std::move(gate_arrays.second); - const auto num_params = expected_num_params(parameter_mapping); + local.parameter_mapping = std::move(gate_arrays.first); + local.gen_coeffs = std::move(gate_arrays.second); + const auto num_params = expected_num_params(local.parameter_mapping); // Nothing here needs a dense state: energy only dots it against the evolved operator, and the gradient // scatters it into its own thread-local scratch before back-evolving. So Heisenberg hands over just the // sparse scores; Schrödinger's state is the live evolved vector, snapshotted whole. - // EvalState owns its rows and snapshots the term count -- a later append push_backs onto the operator's - // sparse rows, which would both dangle a view and outrun the `op` captured below. const auto num_terms = mp_op_.size(); - auto state = [&] { + local.state = [&] { if (schrodinger_) { return EvalState::dense(mp_op_.dense_state()); } const auto sparse = mp_op_.sparse_state(); return EvalState::sparse(num_terms, sparse.rows, sparse.values); }(); - VecD op = mp_op_.get_operator(); - const auto core_term = this->core_term(); - const auto comm = comm_; - - const auto expected_layers = graph_layers(); - // Aliased rather than copied: the check below needs the live counter, like graph->layers(). - const auto *epoch = &initial_operator_epoch_; - const auto expected_epoch = initial_operator_epoch_; + local.op = mp_op_.get_operator(); + local.core_term = this->core_term(); + local.comm = comm_; + + local.expected_graph_layers = graph_layers(); + local.initial_operator_epoch = &initial_operator_epoch_; + local.expected_initial_operator_epoch = initial_operator_epoch_; const auto &inverted_index = mp_op_.inverted_index(); - // One owning handle either way: pare hands back a heap-owned MPGraph the functional must keep alive - // (build_cos_callbacks holds pointers into its layers' stored cos); non-pare aliases graph_. - std::shared_ptr graph; if (pare_threshold.has_value()) { auto full_cos_of_layer = [this, &inverted_index](size_t i) -> CosMask { const auto layer = graph_.get_layer_traversal(i); @@ -958,77 +966,31 @@ auto MonomialPropagator::make_functional_(Fn &&func, std::optional(combined); }; // Threshold the picture's driving vector: the Hamiltonian in Schrödinger, the state otherwise. - const auto keep = schrodinger_ ? indices_above(op, *pare_threshold) : state.indices_above(*pare_threshold); - const auto count = schrodinger_ ? op.size() : state.length(); - graph = + const auto keep = + schrodinger_ ? indices_above(local.op, *pare_threshold) : local.state.indices_above(*pare_threshold); + const auto count = schrodinger_ ? local.op.size() : local.state.length(); + local.graph = std::make_shared(pare_graph(graph_, keep, count, schrodinger_, comm_, full_cos_of_layer)); } else { - graph = std::shared_ptr(std::shared_ptr{}, &graph_); + local.graph = std::shared_ptr(std::shared_ptr{}, &graph_); } - // The folds keep raw column pointers into this propagator's inverted index, so the returned callable - // must not outlive the propagator. - auto cos = build_cos_callbacks(inverted_index, graph->replay_view(), basis_); - - return [func = std::move(func), - core_term, - state = std::move(state), - op = std::move(op), - graph = std::move(graph), - parameter_mapping, - gen_coeffs, - num_params, - epoch, - expected_epoch, - expected_layers, - cos = std::move(cos), - comm](const VecD ¶ms) -> R { - validate_expected_initial_operator(*epoch, expected_epoch); - validate_functional_call(params, num_params); - validate_expected_graph_layers(graph->layers(), expected_layers); - return func(EvalRequest{.e_core = core_term, - .state = state, - .op = op, - .parameter_mapping = parameter_mapping, - .gen_coeffs = gen_coeffs, - .graph = graph->replay_view(), - .params = params}, - comm, - cos); - }; + local.cos = build_cos_callbacks(inverted_index, local.graph->replay_view(), basis_); + + return std::make_shared(num_params, std::move(local)); } template auto MonomialPropagator::expectation_value_functional(std::optional pare_threshold) - -> std::function { - if (partition_group_) { - // Each partition allreduces internally, so partition 0 is the global value. The group is captured by - // raw pointer, so the returned callable must not outlive this propagator. - auto fns = std::make_shared>>( - map_partitions_([&](MonomialPropagator &s) { return s.expectation_value_functional(pare_threshold); })); - auto *grp = partition_group_.get(); - return [grp, fns](const VecD ¶ms) -> double { - return detail::partition::collect_on_all(*grp, - [&](int r) { return (*fns)[static_cast(r)](params); })[0]; - }; - } - return make_functional_(ev_fn, pare_threshold); + -> ExpectationValueFunctional { + return ExpectationValueFunctional(make_plan_(pare_threshold)); } template auto MonomialPropagator::expectation_value_and_gradient_functional(std::optional pare_threshold) - -> std::function(const VecD &)> { - if (partition_group_) { - auto fns = std::make_shared(const VecD &)>>>(map_partitions_( - [&](MonomialPropagator &s) { return s.expectation_value_and_gradient_functional(pare_threshold); })); - auto *grp = partition_group_.get(); - return [grp, fns](const VecD ¶ms) -> std::pair { - return detail::partition::collect_on_all(*grp, - [&](int r) { return (*fns)[static_cast(r)](params); })[0]; - }; - } - return make_functional_(ev_and_grad_fn, pare_threshold); + -> ExpectationValueAndGradientFunctional { + return ExpectationValueAndGradientFunctional(make_plan_(pare_threshold)); } template diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 8ea61cd7..30c4c74e 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -333,3 +333,15 @@ BOOST_AUTO_TEST_CASE(pared_functional_silently_survives_build_graph) { BOOST_TEST(prop.expectation_value(kBaseParams) != before); BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); } + +// The functional objects report the axis they were built against, so a caller can size its parameter +// vector without going back to the propagator. +BOOST_AUTO_TEST_CASE(functional_reports_its_parameter_axis) { + auto prop = make_propagator(/*schrodinger=*/false); + BOOST_TEST(prop.expectation_value_functional().num_params() == 0U); + + build_base_graph(prop); + BOOST_TEST(prop.expectation_value_functional().num_params() == kBaseParams.size()); + BOOST_TEST(prop.expectation_value_and_gradient_functional().num_params() == kBaseParams.size()); + BOOST_TEST(prop.expectation_value_functional(kPareThreshold).num_params() == kBaseParams.size()); +} diff --git a/src/monoprop/bindings/binder.h b/src/monoprop/bindings/binder.h index 5728aca5..87740aa5 100644 --- a/src/monoprop/bindings/binder.h +++ b/src/monoprop/bindings/binder.h @@ -57,6 +57,28 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { auto name = std::format("MonomialPropagator{:03d}", NumModes); auto cls = nb::class_>(mod, name.c_str()); + // The functional objects. Opaque and non-constructible from Python: the only way to one is the + // matching factory below, which keep_alive-pins the propagator it borrows from. + auto ev_name = std::format("ExpectationValueFunctional{:03d}", NumModes); + nb::class_>(mod, ev_name.c_str()) + .def("__call__", + &ExpectationValueFunctional::operator(), + "parameters"_a, + "Expectation value at the given variational parameters") + .def_prop_ro("num_params", + &ExpectationValueFunctional::num_params, + "Parameter-axis length this functional was built against"); + + auto grad_name = std::format("ExpectationValueAndGradientFunctional{:03d}", NumModes); + nb::class_>(mod, grad_name.c_str()) + .def("__call__", + &ExpectationValueAndGradientFunctional::operator(), + "parameters"_a, + "(expectation value, gradient) at the given variational parameters") + .def_prop_ro("num_params", + &ExpectationValueAndGradientFunctional::num_params, + "Parameter-axis length this functional was built against"); + cls.def( "__init__", [](MonomialPropagator *t, @@ -134,14 +156,18 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { "parameters"_a, "Expectation value and its gradient at the given variational parameters"); + // keep_alive<0, 1>: the functional borrows this propagator's inverted index and, without a pare + // threshold, its graph, so the propagator must outlive it. Python has no other way to know. cls.def("expectation_value_functional", &MonomialPropagator::expectation_value_functional, "pare_threshold"_a = std::nullopt, + nb::keep_alive<0, 1>(), "Reusable callable giving the expectation value from parameters; None keeps the exact graph"); cls.def("expectation_value_and_gradient_functional", &MonomialPropagator::expectation_value_and_gradient_functional, "pare_threshold"_a = std::nullopt, + nb::keep_alive<0, 1>(), "Reusable callable giving (expectation value, gradient) from parameters; None keeps the exact graph"); cls.def("contract_partially", diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index ca18c4c7..3ad476d3 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -440,6 +440,22 @@ def call(): else: assert call() == pytest.approx(before), f"{method}: {rationale}" + @pytest.mark.parametrize("partitions", ["off", "auto"]) + @pytest.mark.parametrize( + "factory", + [ + "expectation_value_functional", + "expectation_value_and_gradient_functional", + ], + ) + def test_bound_functional_reports_its_parameter_axis( + self, monkeypatch, serial_comm, factory, partitions + ): + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) + functional = getattr(mp._simulator, factory)(None) + assert functional.num_params == len(self._PARAMS) + @pytest.mark.parametrize("partitions", ["off", "auto"]) def test_parameter_mapping_silently_desynchronises_functional( self, monkeypatch, serial_comm, partitions From 43c57fd8508343da898448f33514d501a0da372c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 11:42:00 +0000 Subject: [PATCH 03/14] fix(propagator): :bug: one control block for functional validity 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 --- AGENTS.md | 16 ++- cpp/include/monoprop/Functional.h | 53 +++++++--- cpp/include/monoprop/MonomialPropagator.h | 18 +++- cpp/monoprop/Validation.cpp | 32 +++--- cpp/monoprop/Validation.h | 18 +++- cpp/monoprop/detail/CMakeLists.txt | 1 + cpp/monoprop/detail/functional/CMakeLists.txt | 8 ++ cpp/monoprop/detail/functional/Control.h | 46 +++++++++ .../MonomialPropagator.inl | 42 ++++++-- cpp/tests/functional_validity.cpp | 99 ++++++++++++++----- cpp/tests/validation_tests.cpp | 28 +++++- tests/test_parameter_validation.py | 51 +++++----- 12 files changed, 311 insertions(+), 101 deletions(-) create mode 100644 cpp/monoprop/detail/functional/CMakeLists.txt create mode 100644 cpp/monoprop/detail/functional/Control.h diff --git a/AGENTS.md b/AGENTS.md index 1798d628..27af1c40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -102,6 +102,15 @@ Key files: (`MajoranaAlgebra`, `PauliAlgebra` in `algebra/Algebra.h`) over shared structural primitives (`algebra/AlgebraCommon.h`). The propagation backbone (the scan/fold in `detail/evolution/...`) is templated on the algebra policy and bound to a runtime `Basis` once, via `with_algebra`. +- **`detail::FunctionalControl`** (`cpp/monoprop/detail/functional/Control.h`): the validity block a + propagator shares with every functional plan it makes — a structure revision, an alive flag, and the + name of the last structural change. A plan borrows from its propagator, so this is how it answers "is + the propagator still there, and does it still hold what I replay?" without dereferencing it. **Every + new mutating method must call `bump_structure_("its_name()")`** once the mutation has committed (a + rejected mutation must not bump); the settings that only gate the next build — the atols, the cutoff, + the cutoff type, the basis change — deliberately do not. A plan additionally re-derives the operator's + store pointer and inverted-index row count as a backstop, so a missing bump reports staleness instead + of folding a rebuilt index. - **The partition facade**: `partitions > 1` makes a `MonomialPropagator` a facade over S single-partition propagators, one hash partition each. Every method that fans out must use the private partition vocabulary declared in `MonomialPropagator.h` (`for_each_partition_`, `map_partitions_`, `concat_partitions_` @@ -176,9 +185,10 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) 7. Add Python bindings in `src/monoprop/bindings/binder.h` 8. Regenerate bindings with `tools/generate-binders.py` 9. Test with both C++ and Python tests -10. If the new method mutates a `MonomialPropagator`, bump - `MonomialPropagator::num_mutating_methods` and add its row to the mutation table (see "Testing - Structure"): a mutator with no row leaves its effect on a live functional unrecorded. +10. If the new method mutates a `MonomialPropagator`: call `bump_structure_` from it (see + `detail::FunctionalControl`), bump `MonomialPropagator::num_mutating_methods`, and add its row to + the mutation table (see "Testing Structure"). A mutator with no row leaves its effect on a live + functional unrecorded. ## Documentation Maintenance Policy diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index bc71a20d..a76b01e7 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -26,7 +26,9 @@ #include "monoprop/TypeAliases.h" #include "monoprop/Validation.h" #include "monoprop/detail/evolution/CosineRecomputeCallbacks.h" +#include "monoprop/detail/functional/Control.h" #include "monoprop/detail/mpi/Comm.h" +#include "monoprop/detail/operator/MPOperator.h" #include "monoprop/detail/partition/PartitionGroup.h" namespace monoprop { @@ -63,11 +65,13 @@ class FunctionalPlan { CosCallbacks cos; mpi::Comm comm{}; ///< real MPI across nodes, or the in-process comm across partitions - // Validity, checked before every replay. `initial_operator_epoch` is aliased rather than copied - // because the check needs the propagator's live counter, as `graph->layers()` needs its live graph. - const size_t *initial_operator_epoch{nullptr}; - size_t expected_initial_operator_epoch{0}; - size_t expected_graph_layers{0}; + // The operator-layout backstop. `mp_op` is borrowed and read only after the alive flag says the + // propagator is still there; the other two are what the borrowed inverted index was built over. + // Compared before any use of that index, so a mutation that forgot its revision bump still + // reports staleness rather than folding a rebuilt index through a pointer to the old one. + const MPOperator *mp_op{nullptr}; + const OperatorIndex *op_store{nullptr}; + size_t inverted_index_rows{0}; }; /// A partition facade's snapshot: one child plan per partition, replayed together. @@ -77,30 +81,41 @@ class FunctionalPlan { std::vector> partitions; ///< in partition order }; - FunctionalPlan(size_t num_params, Local local) : num_params_(num_params), shape_(std::move(local)) {} + /// `control` is the propagator's own block; the plan pins the revision it is built at. + FunctionalPlan(size_t num_params, std::shared_ptr control, Local local) + : num_params_(num_params), + control_(std::move(control)), + expected_revision_(control_->structure_revision.load()), + shape_(std::move(local)) {} - FunctionalPlan(size_t num_params, Fanout fanout) : num_params_(num_params), shape_(std::move(fanout)) {} + FunctionalPlan(size_t num_params, std::shared_ptr control, Fanout fanout) + : num_params_(num_params), + control_(std::move(control)), + expected_revision_(control_->structure_revision.load()), + shape_(std::move(fanout)) {} /// The parameter-axis length the plan was built against; a call must supply exactly this many. auto num_params() const -> size_t { return num_params_; } /// Throw unless `params` fits and the propagator still holds what the plan replays. - // A facade validates nothing here: the state each check reads lives on the partitions, so each child - // plan checks itself on its own master thread, inside evaluate(). + // A facade checks its own control block here -- the group it fans out over belongs to the facade -- + // and each child plan then checks its own partition's, on that partition's master thread. Only the + // single-partition shape has an operator to run the layout backstop against. auto validate(const VecD ¶ms) const -> void { const auto *local = std::get_if(&shape_); - if (local == nullptr) { - return; - } - validate_expected_initial_operator(*local->initial_operator_epoch, local->expected_initial_operator_epoch); + validate_functional_state({.propagator_alive = control_->propagator_alive.load(), + .current_revision = control_->structure_revision.load(), + .expected_revision = expected_revision_, + .operator_layout_unchanged = local == nullptr || operator_layout_unchanged(*local), + .last_structural_change = control_->last_structural_change.load()}); validate_functional_call(params, num_params_); - validate_expected_graph_layers(local->graph->layers(), local->expected_graph_layers); } /// Replay the snapshot: `fn(request, comm, cos)` locally, or partition 0's answer on a facade. template > auto evaluate(Fn &&fn, const VecD ¶ms) const -> R { + validate(params); if (const auto *fanout = std::get_if(&shape_)) { // Each partition allreduces internally, so partition 0 already carries the global answer. // The fan-out must reach every master: the partitions' collectives are barrier-synced. @@ -108,7 +123,6 @@ class FunctionalPlan { return fanout->partitions[static_cast(r)]->evaluate(fn, params); })[0]; } - validate(params); const auto &local = std::get(shape_); return fn(EvalRequest{.e_core = local.core_term, .state = local.state, @@ -122,7 +136,16 @@ class FunctionalPlan { } private: + // Read straight off the borrowed operator, never through inverted_index(): that accessor rebuilds a + // stale index, which is a write, and a plan must not write to its propagator. + static auto operator_layout_unchanged(const Local &local) -> bool { + return local.mp_op->store.get() == local.op_store && local.mp_op->inverted_index_.has_value() + && local.mp_op->inverted_index_->rows() == local.inverted_index_rows; + } + size_t num_params_{0}; + std::shared_ptr control_; + size_t expected_revision_{0}; std::variant shape_; }; diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index 0eb35451..82c61668 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -309,10 +309,6 @@ class MonomialPropagator { std::optional lower_atol_, upper_atol_; double core_term_{0.0}; - // Bumped by every initial-operator re-weight. A functional snapshots the operator coefficients, so - // it captures this and rejects a later call once it moves, as it does for a rebuilt graph. - size_t initial_operator_epoch_{0}; - size_t logical_num_modes_{NumModes}; CutoffType cutoff_type_; @@ -327,6 +323,20 @@ class MonomialPropagator { // PartitionGroup rebinds a cloned partition's comm_ to its own transport during a deep copy. friend class detail::partition::PartitionGroup; + // Shared with every functional plan this propagator makes: they read it, only this propagator writes + // it. A copy gets its own block, because a copy carries no functionals. + std::shared_ptr functional_control_{std::make_shared()}; + + // Record that what a plan replays has moved. `site` must be a string literal: plans read it after + // this propagator is gone. A mutation that is rejected outright must not bump -- it changed nothing, + // so it must not invalidate anything. A mutation that can fail part-way bumps before it starts + // instead: invalidating a functional that did not need it costs a rebuild, answering from a snapshot + // of a half-written operator costs a wrong number. + auto bump_structure_(const char *site) -> void { + functional_control_->last_structural_change.store(site); + functional_control_->structure_revision.fetch_add(1); + } + // A facade's own graph_/mp_op_ are never populated, so handing them out would return plausible-looking // empty state; there is no meaningful merge either, since the callers want one partition's raw layout. auto require_single_partition_(const char *what) const -> void { diff --git a/cpp/monoprop/Validation.cpp b/cpp/monoprop/Validation.cpp index 46e5252d..55113573 100644 --- a/cpp/monoprop/Validation.cpp +++ b/cpp/monoprop/Validation.cpp @@ -99,20 +99,26 @@ auto validate_functional_call(const VecD ¶meters, size_t expected_num_params } } -auto validate_expected_graph_layers(size_t current_layers, size_t expected_layers) -> void { - if (current_layers != expected_layers) { - throw StaleFunctionalGraph(std::format("MP object has been modified since the functional was created. " - "Previous number of graph layers was {} and now is {}.", - expected_layers, - current_layers)); +auto validate_functional_state(const FunctionalState &state) -> void { + // Aliveness first: every other handle a functional holds points into the propagator, so there is + // nothing else it may legally read once this is false. + if (!state.propagator_alive) { + throw StaleFunctionalGraph("The propagator this functional was built from has been destroyed. " + "A functional reads the propagator's operator index directly, so it " + "cannot outlive it; keep the propagator alive, or build the functional " + "again from a live one."); } -} - -auto validate_expected_initial_operator(size_t current_epoch, size_t expected_epoch) -> void { - if (current_epoch != expected_epoch) { - throw StaleFunctionalGraph("MP object has been modified since the functional was created. " - "The initial operator was re-weighted, so the coefficients the functional " - "snapshotted are stale; create a new functional."); + if (state.current_revision != state.expected_revision) { + throw StaleFunctionalGraph(std::format( + "MP object has been modified since the functional was created: {} changed the " + "graph or operator the functional replays. Create a new functional.", + state.last_structural_change != nullptr ? state.last_structural_change : "a structural mutation")); + } + // No bump, but the operator moved anyway: some mutation is missing its bump_structure_ call. Report + // it as staleness rather than read a rebuilt inverted index through a pointer to the old one. + if (!state.operator_layout_unchanged) { + throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " + "operator index it reads was rebuilt or grown. Create a new functional."); } } diff --git a/cpp/monoprop/Validation.h b/cpp/monoprop/Validation.h index db93e622..5f75ae0c 100644 --- a/cpp/monoprop/Validation.h +++ b/cpp/monoprop/Validation.h @@ -14,6 +14,7 @@ #pragma once +#include #include #include @@ -38,11 +39,18 @@ monoprop_EXPORT auto validate_parameters_length(const VecD ¶ms, const VecZ & monoprop_EXPORT auto validate_functional_call(const VecD ¶meters, size_t expected_num_params) -> void; -// The graph must still have the layer count the functional was built against. -monoprop_EXPORT auto validate_expected_graph_layers(size_t current_layers, size_t expected_layers) -> void; - -// The initial operator must not have been re-weighted since the functional snapshotted its coefficients. -monoprop_EXPORT auto validate_expected_initial_operator(size_t current_epoch, size_t expected_epoch) -> void; +/// What a functional must be able to say about its propagator before it reads anything it borrowed. +// Assembled from detail::FunctionalControl plus, for a single-partition plan, two facts derived from the +// operator itself. The derived pair is the backstop: it holds even for a mutation that forgot to bump. +struct FunctionalState { + bool propagator_alive; ///< false once ~MonomialPropagator has run + size_t current_revision; ///< the propagator's structure revision now + size_t expected_revision; ///< the revision the functional was built at + bool operator_layout_unchanged; ///< the borrowed inverted index still spans the same store and rows + const char *last_structural_change; ///< the method that last bumped the revision, or nullptr +}; + +monoprop_EXPORT auto validate_functional_state(const FunctionalState &state) -> void; // only_rotate_len_k is optional; when set it must satisfy 0 < k <= max_k. monoprop_EXPORT auto validate_only_rotate_len_k_(std::optional only_rotate_len_k, size_t max_k) -> void; diff --git a/cpp/monoprop/detail/CMakeLists.txt b/cpp/monoprop/detail/CMakeLists.txt index f75f51c5..d075e550 100644 --- a/cpp/monoprop/detail/CMakeLists.txt +++ b/cpp/monoprop/detail/CMakeLists.txt @@ -8,6 +8,7 @@ target_sources( ) add_subdirectory(evolution) +add_subdirectory(functional) add_subdirectory(graph) add_subdirectory(graph_encoding) add_subdirectory(monomial_propagator) diff --git a/cpp/monoprop/detail/functional/CMakeLists.txt b/cpp/monoprop/detail/functional/CMakeLists.txt new file mode 100644 index 00000000..a8b518aa --- /dev/null +++ b/cpp/monoprop/detail/functional/CMakeLists.txt @@ -0,0 +1,8 @@ +target_sources( + monoprop + PUBLIC + FILE_SET headers + TYPE HEADERS + FILES + "Control.h" +) diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h new file mode 100644 index 00000000..885da5ff --- /dev/null +++ b/cpp/monoprop/detail/functional/Control.h @@ -0,0 +1,46 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include + +namespace monoprop::detail { + +// The validity block a propagator shares with every functional plan it makes. +// +// A plan borrows from its propagator, so before it reads any of those handles it needs two facts its +// own snapshot cannot supply: whether the propagator is still there, and whether the structure the +// snapshot describes is still the propagator's. Both live here, behind one shared_ptr, so a plan +// answers them without dereferencing the propagator at all. The propagator holds the only mutating +// handle; plans hold shared_ptr. +// +// A copied propagator gets its own block: a copy carries no functionals, so it starts at revision 0. +struct FunctionalControl { + // Bumped by every change to what a plan replays -- the graph's layers, their parameter labels, or + // the operator's rows. Deliberately NOT bumped by the settings that only gate the next build (the + // atols, the cutoff, the cutoff type, the basis change): none of them touches a plan's snapshot. + std::atomic structure_revision{0}; + + // Cleared by ~MonomialPropagator, which runs before its members go away. A plan that outlives its + // propagator must report that instead of reading through handles into freed memory. + std::atomic propagator_alive{true}; + + // The method that last bumped structure_revision, so the error can name it. Always a string + // literal, whose lifetime outlives every propagator. + std::atomic last_structural_change{nullptr}; +}; + +} // namespace monoprop::detail diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 442f689f..4294a28c 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -188,7 +188,12 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope } template -MonomialPropagator::~MonomialPropagator() = default; +MonomialPropagator::~MonomialPropagator() { + // Before any member goes away, so a functional that outlives this propagator reports the destruction + // instead of reading through the handles it borrowed. On a facade this also runs before + // partition_group_ is destroyed, so a fanned-out plan sees the facade dead first. + functional_control_->propagator_alive.store(false); +} template MonomialPropagator::MonomialPropagator(const MonomialPropagator &other) @@ -202,7 +207,6 @@ MonomialPropagator::MonomialPropagator(const MonomialPropagator &other lower_atol_(other.lower_atol_), upper_atol_(other.upper_atol_), core_term_(other.core_term_), - initial_operator_epoch_(other.initial_operator_epoch_), logical_num_modes_(other.logical_num_modes_), cutoff_type_(other.cutoff_type_), basis_change_(other.basis_change_), @@ -351,7 +355,9 @@ auto MonomialPropagator::packed_inline_width_() const -> size_t { template auto MonomialPropagator::apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD> { - ++initial_operator_epoch_; + // Up front, not at the end: the distribution loop below can throw part-way (a rejected term, an + // out-of-range index) with core_term_ already written, and a facade can have some partitions applied. + bump_structure_("update_initial_operator()"); if (partition_group_) { // The facade holds no local terms of its own, so the return is empty. for_each_partition_([&](MonomialPropagator &s) { s.update_initial_operator(op_dict); }); @@ -629,6 +635,7 @@ auto MonomialPropagator::build_graph(const std::vector &majorana for_each_partition_([&](MonomialPropagator &s) { s.build_graph(majoranas, parameter_mapping, gen_coeffs, gate_indices, parameters, only_rotate_len_k); }); + bump_structure_("build_graph()"); return; } if (majoranas.empty()) { @@ -687,6 +694,7 @@ auto MonomialPropagator::build_graph(const std::vector &majorana seed, only_rotate_len_k); } + bump_structure_("build_graph()"); } template @@ -700,6 +708,7 @@ auto MonomialPropagator::propagate(const std::vector &majoranas, for_each_partition_([&](MonomialPropagator &s) { s.propagate(majoranas, parameter_mapping, gen_coeffs, parameters, only_rotate_len_k); }); + bump_structure_("propagate()"); return; } if (majoranas.empty()) { @@ -715,6 +724,7 @@ auto MonomialPropagator::propagate(const std::vector &majoranas, graph_layers())); } evolve_mode_contract_immediately_(majoranas, parameter_mapping, gen_coeffs, parameters, only_rotate_len_k); + bump_structure_("propagate()"); } template @@ -810,6 +820,7 @@ template auto MonomialPropagator::set_parameter_mapping(const VecZ ¶meter_mapping) -> void { if (partition_group_) { for_each_partition_([&](MonomialPropagator &s) { s.set_parameter_mapping(parameter_mapping); }); + bump_structure_("set_parameter_mapping()"); return; } const size_t count = graph_.layers(); @@ -849,6 +860,7 @@ auto MonomialPropagator::set_parameter_mapping(const VecZ ¶meter_m count, gates)); } + bump_structure_("set_parameter_mapping()"); } template @@ -928,7 +940,9 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho fanout.partitions = map_partitions_([&](MonomialPropagator &s) { return s.make_plan_(pare_threshold); }); // graph_gate_arrays_() reads partition 0: the graph structure and gate info are identical on // every partition. A facade validates nothing itself (see FunctionalPlan::validate). - return std::make_shared(expected_num_params(graph_gate_arrays_().first), std::move(fanout)); + return std::make_shared(expected_num_params(graph_gate_arrays_().first), + functional_control_, + std::move(fanout)); } typename Plan::Local local; @@ -953,10 +967,10 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.core_term = this->core_term(); local.comm = comm_; - local.expected_graph_layers = graph_layers(); - local.initial_operator_epoch = &initial_operator_epoch_; - local.expected_initial_operator_epoch = initial_operator_epoch_; const auto &inverted_index = mp_op_.inverted_index(); + local.mp_op = &mp_op_; + local.op_store = mp_op_.store.get(); + local.inverted_index_rows = inverted_index.rows(); if (pare_threshold.has_value()) { auto full_cos_of_layer = [this, &inverted_index](size_t i) -> CosMask { @@ -978,7 +992,7 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.cos = build_cos_callbacks(inverted_index, local.graph->replay_view(), basis_); - return std::make_shared(num_params, std::move(local)); + return std::make_shared(num_params, functional_control_, std::move(local)); } template @@ -1014,7 +1028,12 @@ auto MonomialPropagator::expectation_value_and_gradient(const VecD &pa template auto MonomialPropagator::contract_partially(const VecD ¶meters, bool inplace) -> VecD { if (partition_group_) { - return concat_partitions_([&](MonomialPropagator &s) { return s.contract_partially(parameters, inplace); }); + auto merged = + concat_partitions_([&](MonomialPropagator &s) { return s.contract_partially(parameters, inplace); }); + if (inplace) { + bump_structure_("contract_partially(inplace=true)"); + } + return merged; } const auto gate_arrays = graph_gate_arrays_(); const auto ¶meter_mapping = gate_arrays.first; @@ -1022,6 +1041,7 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo validate_parameters_length(parameters, parameter_mapping); if (parameters.empty()) { + // Nothing is folded and the graph is left whole, so this is not a mutation even when inplace. return current_picture_coeffs_(); } @@ -1036,6 +1056,9 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo const MPGraph sliced = graph_.slice_graph(num_majoranas, true); evolved_state = evolve_operator_with_recompute_(VecD(state), sliced.replay_view(), mapped_params); mp_op_.state_coeffs = evolved_state; + // The graph lost the layers it folded and the state moved, so every plan built against + // either is stale. Out of place nothing changes, so there is no bump on that branch. + bump_structure_("contract_partially(inplace=true)"); } else { evolved_state = @@ -1051,6 +1074,7 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo const MPGraph sliced = graph_.slice_graph(num_majoranas, true); evolved_op = evolve_operator_with_recompute_(VecD(op), sliced.replay_view(), mapped_params); mp_op_.op_coeffs = evolved_op; + bump_structure_("contract_partially(inplace=true)"); } else { evolved_op = evolve_operator_with_recompute_(VecD(op), graph_.slice_view(num_majoranas), mapped_params); diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 30c4c74e..3f6cca8e 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -47,7 +48,8 @@ const VecD kBaseParams{0.3, 0.7}; // symmetric under swapping the two angles -- which is what makes the set_parameter_mapping row bite. const std::vector kBaseGates{VecZ{0}, VecZ{2}}; -auto make_propagator(bool schrodinger) -> Prop { +// `partitions` is passed explicitly, so it wins over the suite-wide monoprop_PARTITIONS=off. +auto make_propagator(bool schrodinger, size_t partitions = 1) -> Prop { OperatorDict initial_ham; initial_ham[VecZ{0, 1}] = std::complex{0.0, 1.0}; initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; @@ -60,7 +62,10 @@ auto make_propagator(bool schrodinger) -> Prop { std::nullopt, std::nullopt, CutoffType::Support, - std::nullopt); + std::nullopt, + kNumModes, + Basis::Majorana, + partitions); } auto build_base_graph(Prop &prop) -> void { @@ -135,36 +140,37 @@ constexpr std::array kMutatorTable{ .apply = &mutate_build_graph, .needs_empty_graph = false, .exact = Outcome::Stale, - .pared = Outcome::Answers, - .rationale = "DEFECT (fixed in stage 2): the pared plan owns its layers, so its layer " - "count cannot move and the live-graph check never fires."}, + .pared = Outcome::Stale, + .rationale = "Appending a layer moves the structure revision, which a pared plan reads as " + "readily as an exact one."}, MutatorRow{.method = "propagate", .apply = &mutate_propagate, .needs_empty_graph = true, - .exact = Outcome::Answers, - .pared = Outcome::Answers, - .rationale = "DEFECT (fixed in stage 2): propagate() leaves the layer count at zero and " - "does not touch the epoch, so neither check sees the re-evolved operator."}, + .exact = Outcome::Stale, + .pared = Outcome::Stale, + .rationale = "Re-evolves the operator in place. It leaves the layer count at zero, which is " + "why a layer count was never enough to see it."}, MutatorRow{.method = "contract_partially", .apply = &mutate_contract_partially, .needs_empty_graph = false, .exact = Outcome::Stale, - .pared = Outcome::Answers, - .rationale = "DEFECT (fixed in stage 2): as build_graph, the pared plan's layer count is fixed."}, + .pared = Outcome::Stale, + .rationale = "Consumes the folded layers and rewrites the coefficients. Only inplace=true " + "bumps; see contract_partially_out_of_place_keeps_functional_valid."}, MutatorRow{.method = "update_initial_operator", .apply = &mutate_update_initial_operator, .needs_empty_graph = false, .exact = Outcome::Stale, .pared = Outcome::Stale, - .rationale = "The epoch check fires. Stage 4 turns this into a weight refresh, except for " - "a pared Schrodinger plan."}, + .rationale = "A re-weight bumps the revision. Stage 4 turns this into a weight refresh, " + "except for a pared Schrodinger plan."}, MutatorRow{.method = "set_parameter_mapping", .apply = &mutate_set_parameter_mapping, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, - .rationale = "DEFECT (fixed in stage 2): relabelling is in place, so the layer count and " - "the epoch both hold and the plan keeps replaying the old labels."}, + .exact = Outcome::Stale, + .pared = Outcome::Stale, + .rationale = "Relabels the layers in place, which changes neither the layer count nor the " + "operator -- the revision is the only thing that sees it."}, MutatorRow{.method = "update_cutoff", .apply = &mutate_update_cutoff, .needs_empty_graph = false, @@ -306,11 +312,11 @@ BOOST_AUTO_TEST_CASE(propagate_on_non_empty_graph_leaves_functional_valid) { BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); } -// The two Answers-with-a-defect rows above only say the number did not move. These two say why that -// is wrong: the propagator's own answer *did* move, so the functional is now reporting a circuit -// nobody asked about. Stage 2 turns both calls into throws. +// The two rows those tests cover used to answer instead of throwing, each returning a number for a +// circuit nobody had asked about any more. These say so directly: the propagator's own answer moves, +// and the functional refuses rather than following it half way. -BOOST_AUTO_TEST_CASE(set_parameter_mapping_silently_desynchronises_functional) { +BOOST_AUTO_TEST_CASE(set_parameter_mapping_invalidates_functional_it_desynchronises) { auto prop = make_propagator(/*schrodinger=*/false); build_base_graph(prop); auto call = make_call(prop, /*gradient=*/false, std::nullopt); @@ -319,10 +325,10 @@ BOOST_AUTO_TEST_CASE(set_parameter_mapping_silently_desynchronises_functional) { mutate_set_parameter_mapping(prop); BOOST_TEST(prop.expectation_value(kBaseParams) != before); - BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); + BOOST_CHECK_THROW(call(kBaseParams), std::runtime_error); } -BOOST_AUTO_TEST_CASE(pared_functional_silently_survives_build_graph) { +BOOST_AUTO_TEST_CASE(pared_functional_is_invalidated_by_build_graph) { auto prop = make_propagator(/*schrodinger=*/false); build_base_graph(prop); auto call = make_call(prop, /*gradient=*/false, kPareThreshold); @@ -331,7 +337,22 @@ BOOST_AUTO_TEST_CASE(pared_functional_silently_survives_build_graph) { mutate_build_graph(prop); BOOST_TEST(prop.expectation_value(kBaseParams) != before); - BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); + BOOST_CHECK_THROW(call(kBaseParams), std::runtime_error); +} + +// A functional reads its propagator's operator index directly, so it cannot outlive it. The control +// block records the destruction, which is the only thing left to read once the handles are dangling. +BOOST_AUTO_TEST_CASE(functional_reports_a_destroyed_propagator) { + auto prop = std::make_unique(make_propagator(/*schrodinger=*/false)); + build_base_graph(*prop); + auto call = make_call(*prop, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + + prop.reset(); + + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, [](const std::runtime_error &e) { + return std::string_view(e.what()).find("has been destroyed") != std::string_view::npos; + }); } // The functional objects report the axis they were built against, so a caller can size its parameter @@ -345,3 +366,33 @@ BOOST_AUTO_TEST_CASE(functional_reports_its_parameter_axis) { BOOST_TEST(prop.expectation_value_and_gradient_functional().num_params() == kBaseParams.size()); BOOST_TEST(prop.expectation_value_functional(kPareThreshold).num_params() == kBaseParams.size()); } + +// A facade's plan holds the partition group by raw pointer, so it needs the facade's own control block: +// the children cannot report a destruction that takes their group with it. +BOOST_AUTO_TEST_CASE(fanned_out_functional_reports_a_destroyed_facade) { + auto facade = std::make_unique(make_propagator(/*schrodinger=*/false, /*partitions=*/2)); + build_base_graph(*facade); + auto call = make_call(*facade, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + + facade.reset(); + + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, [](const std::runtime_error &e) { + return std::string_view(e.what()).find("has been destroyed") != std::string_view::npos; + }); +} + +// The facade bumps its own revision as it fans a mutation out, so the error is reported on the calling +// thread rather than surfacing out of a partition's collective. +BOOST_AUTO_TEST_CASE(fanned_out_functional_is_invalidated_by_build_graph) { + auto facade = make_propagator(/*schrodinger=*/false, /*partitions=*/2); + build_base_graph(facade); + auto call = make_call(facade, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + + mutate_build_graph(facade); + + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, [](const std::runtime_error &e) { + return std::string_view(e.what()).find("build_graph()") != std::string_view::npos; + }); +} diff --git a/cpp/tests/validation_tests.cpp b/cpp/tests/validation_tests.cpp index 173c3a2d..3f784bca 100644 --- a/cpp/tests/validation_tests.cpp +++ b/cpp/tests/validation_tests.cpp @@ -17,6 +17,7 @@ #include #include +#include #include "monoprop/TypeAliases.h" #include "monoprop/Validation.h" @@ -56,9 +57,30 @@ BOOST_AUTO_TEST_CASE(validation_functional_call) { BOOST_CHECK_THROW(validate_functional_call(VecD{0.1}, 2), std::runtime_error); } -BOOST_AUTO_TEST_CASE(validation_expected_graph_layers) { - BOOST_CHECK_NO_THROW(validate_expected_graph_layers(3, 3)); - BOOST_CHECK_THROW(validate_expected_graph_layers(4, 3), std::runtime_error); +BOOST_AUTO_TEST_CASE(validation_functional_state) { + const FunctionalState healthy{.propagator_alive = true, + .current_revision = 3, + .expected_revision = 3, + .operator_layout_unchanged = true, + .last_structural_change = nullptr}; + BOOST_CHECK_NO_THROW(validate_functional_state(healthy)); + + auto destroyed = healthy; + destroyed.propagator_alive = false; + BOOST_CHECK_THROW(validate_functional_state(destroyed), std::runtime_error); + + // The revision names the mutation that moved the structure, so the message can point at it. + auto mutated = healthy; + mutated.current_revision = 4; + mutated.last_structural_change = "build_graph()"; + BOOST_CHECK_EXCEPTION(validate_functional_state(mutated), std::runtime_error, [](const auto &e) { + return std::string_view(e.what()).find("build_graph()") != std::string_view::npos; + }); + + // The backstop: the operator moved without a revision bump. + auto rebuilt = healthy; + rebuilt.operator_layout_unchanged = false; + BOOST_CHECK_THROW(validate_functional_state(rebuilt), std::runtime_error); } BOOST_AUTO_TEST_CASE(validation_only_rotate_len_k) { diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index 3ad476d3..dc0b7f06 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -298,26 +298,26 @@ def _mutate_upper_atol(mp): "_mutate_build_graph", False, "stale", - "answers", - "DEFECT (fixed in stage 2): the pared plan owns its layers, so its layer count cannot " - "move and the live-graph check never fires.", + "stale", + "Appending a layer moves the structure revision, which a pared plan reads as readily as " + "an exact one.", ), ( "propagate", "_mutate_propagate", True, - "answers", - "answers", - "DEFECT (fixed in stage 2): propagate() leaves the layer count at zero and does not " - "touch the epoch, so neither check sees the re-evolved operator.", + "stale", + "stale", + "Re-evolves the operator in place. It leaves the layer count at zero, which is why a " + "layer count was never enough to see it.", ), ( "contract_partially", "_mutate_contract_partially", False, "stale", - "answers", - "DEFECT (fixed in stage 2): as build_graph, the pared plan's layer count is fixed.", + "stale", + "Consumes the folded layers and rewrites the coefficients. Only inplace=True bumps.", ), ( "update_initial_operator", @@ -325,17 +325,17 @@ def _mutate_upper_atol(mp): False, "stale", "stale", - "The epoch check fires. Stage 4 turns this into a weight refresh, except for a pared " - "Schrodinger plan.", + "A re-weight bumps the revision. Stage 4 turns this into a weight refresh, except for " + "a pared Schrodinger plan.", ), ( "parameter_mapping", "_mutate_parameter_mapping", False, - "answers", - "answers", - "DEFECT (fixed in stage 2): relabelling is in place, so the layer count and the epoch " - "both hold and the plan keeps replaying the old labels.", + "stale", + "stale", + "Relabels the layers in place, which changes neither the layer count nor the operator -- " + "the revision is the only thing that sees it.", ), ( "cutoff", @@ -457,11 +457,11 @@ def test_bound_functional_reports_its_parameter_axis( assert functional.num_params == len(self._PARAMS) @pytest.mark.parametrize("partitions", ["off", "auto"]) - def test_parameter_mapping_silently_desynchronises_functional( + def test_parameter_mapping_invalidates_functional_it_desynchronises( self, monkeypatch, serial_comm, partitions ): - """Defect 2, made falsifiable: the relabel moves the propagator's own answer while the - functional built before it keeps returning the old one. Stage 2 makes the call throw. + """The relabel moves the propagator's own answer, and the functional refuses rather than + following it half way -- it used to keep returning the pre-relabel number. """ monkeypatch.setenv("monoprop_PARTITIONS", partitions) mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) @@ -472,14 +472,15 @@ def test_parameter_mapping_silently_desynchronises_functional( self._mutate_parameter_mapping(mp) assert mp.expval(parameters) != pytest.approx(before) - assert functional(parameters) == pytest.approx(before) + with pytest.raises(RuntimeError, match=r"set_parameter_mapping"): + functional(parameters) @pytest.mark.parametrize("partitions", ["off", "auto"]) - def test_pared_functional_silently_survives_build_graph( + def test_pared_functional_is_invalidated_by_build_graph( self, monkeypatch, serial_comm, partitions ): - """Defect 1, made falsifiable: appending a layer moves the propagator's own answer while a - pared functional, whose owned layer count cannot change, keeps returning the old one. + """A pared plan owns its layers, so its layer count cannot move; the structure revision is + what sees the appended layer. """ monkeypatch.setenv("monoprop_PARTITIONS", partitions) mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) @@ -489,10 +490,10 @@ def test_pared_functional_silently_survives_build_graph( self._mutate_build_graph(mp) - # The appended gate claims a fresh angle, so the propagator's own axis is now three long - # while the functional still answers for the two-gate circuit it was built against. + # The appended gate claims a fresh angle, so the propagator's own axis is now three long. assert mp.expval([*parameters, 0.4]) != pytest.approx(before) - assert functional(parameters) == pytest.approx(before) + with pytest.raises(RuntimeError, match=r"build_graph"): + functional(parameters) class TestEvolvedOperatorBothPictures: From d946bd24ae3def65027381e3b7fd211c9bce3591 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 11:59:33 +0000 Subject: [PATCH 04/14] fix(propagator): :bug: check the alive flag before the operator backstop 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 --- cpp/include/monoprop/Functional.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index a76b01e7..48a50dc4 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -102,8 +102,13 @@ class FunctionalPlan { // and each child plan then checks its own partition's, on that partition's master thread. Only the // single-partition shape has an operator to run the layout backstop against. auto validate(const VecD ¶ms) const -> void { - const auto *local = std::get_if(&shape_); - validate_functional_state({.propagator_alive = control_->propagator_alive.load(), + // Aliveness is settled here rather than left to validate_functional_state: the layout backstop + // reads the propagator's operator, and every argument is evaluated before the callee runs, so a + // dead propagator has to drop out of the argument list itself. The control block is shared, so it + // stays readable after the propagator is gone -- nothing else the plan holds does. + const bool alive = control_->propagator_alive.load(); + const auto *local = alive ? std::get_if(&shape_) : nullptr; + validate_functional_state({.propagator_alive = alive, .current_revision = control_->structure_revision.load(), .expected_revision = expected_revision_, .operator_layout_unchanged = local == nullptr || operator_layout_unchanged(*local), From fdb0a88a0f255a3622e9a8ceaa3e2d4feba52d26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 12:03:07 +0000 Subject: [PATCH 05/14] refactor(propagator): :recycle: the functional plan owns its layers 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 --- cpp/include/monoprop/Functional.h | 5 ++--- .../monomial_propagator/MonomialPropagator.inl | 12 +++++++++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index 48a50dc4..55541797 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -56,9 +56,8 @@ class FunctionalPlan { VecD op; ///< un-evolved operator coefficients, copied out of the propagator VecZ parameter_mapping; ///< optimizer order: which parameter drives graph layer i VecD gen_coeffs; ///< optimizer order, parallel to parameter_mapping - // One owning handle either way: pare hands back a heap-owned MPGraph the plan must keep alive - // (`cos` holds pointers into its layers' stored cos); non-pare aliases the propagator's graph_ - // through a shared_ptr with an empty owner block, so it stays live only while the propagator does. + // Always owned, never a view on the propagator's graph_: `cos` holds a raw CosMask pointer per + // layer, and only layers the plan owns are safe from a later append, slice or compaction. std::shared_ptr graph; // The folds keep raw column pointers into the propagator's inverted index, so this plan must not // outlive the propagator either. diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 4294a28c..969c34af 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -972,6 +972,11 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.op_store = mp_op_.store.get(); local.inverted_index_rows = inverted_index.rows(); + // The plan always owns its layers, so `cos` -- which holds a raw CosMask pointer per layer -- points + // into layers no later append_layer, slice_graph or maybe_compact_layers can move or free. 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. Only a + // pared graph carries masks, and pare_graph builds those into an owned graph anyway. if (pare_threshold.has_value()) { auto full_cos_of_layer = [this, &inverted_index](size_t i) -> CosMask { const auto layer = graph_.get_layer_traversal(i); @@ -987,7 +992,12 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho std::make_shared(pare_graph(graph_, keep, count, schrodinger_, comm_, full_cos_of_layer)); } else { - local.graph = std::shared_ptr(std::shared_ptr{}, &graph_); + std::vector owned; + owned.reserve(graph_.layers()); + for (size_t i = 0; i < graph_.layers(); ++i) { + owned.push_back(graph_.get_layer(i)); + } + local.graph = std::make_shared(graph_.is_schrodinger(), std::move(owned)); } local.cos = build_cos_callbacks(inverted_index, local.graph->replay_view(), basis_); From bc4172cc2f6aab06577969aada680f93a01d1d2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 14:10:23 +0000 Subject: [PATCH 06/14] feat(propagator)!: :sparkles: a functional follows an initial-operator re-weight `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 --- cpp/include/monoprop/Functional.h | 48 +++- cpp/include/monoprop/MonomialPropagator.h | 13 +- cpp/monoprop/Validation.cpp | 22 ++ cpp/monoprop/Validation.h | 9 + cpp/monoprop/detail/functional/Control.h | 24 ++ .../MonomialPropagator.inl | 90 +++++--- cpp/tests/functional_validity.cpp | 210 ++++++++++++++++-- cpp/tests/update_initial_operator.cpp | 11 +- cpp/tests/validation_tests.cpp | 18 ++ src/monoprop/majorana_propagator.py | 5 +- src/monoprop/monomial_propagator.py | 37 ++- src/monoprop/pauli_propagator.py | 5 +- tests/test_parameter_validation.py | 141 ++++++++++-- 13 files changed, 537 insertions(+), 96 deletions(-) diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index 55541797..53353135 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -49,11 +49,13 @@ class FunctionalPlan { public: /// A single-partition propagator's snapshot: one replay of its graph against its operator. struct Local { - double core_term{0.0}; ///< the identity term, added to the summed expectation value + // The weights this plan was built over, and the fallback when the propagator has published no + // newer set. The same object the control block holds, not a copy of it, so the pointers compare + // equal until a re-weight publishes -- which is how a call sees that it has weights to follow. + std::shared_ptr weights; // Owns its rows and snapshots the term count: the operator's sparse rows grow by push_back as - // terms are appended, so a view would both dangle and outrun `op`. + // terms are appended, so a view would both dangle and outrun the weights' `op`. EvalState state; ///< the contraction partner, sparse (Heisenberg) or dense (Schrodinger) - VecD op; ///< un-evolved operator coefficients, copied out of the propagator VecZ parameter_mapping; ///< optimizer order: which parameter drives graph layer i VecD gen_coeffs; ///< optimizer order, parallel to parameter_mapping // Always owned, never a view on the propagator's graph_: `cos` holds a raw CosMask pointer per @@ -71,6 +73,12 @@ class FunctionalPlan { const MPOperator *mp_op{nullptr}; const OperatorIndex *op_store{nullptr}; size_t inverted_index_rows{0}; + + // Whether `graph`'s keep-set was thresholded from the operator coefficients, which is Schrodinger + // with a pare threshold and nothing else. Such a plan cannot follow a re-weight: the new + // coefficients select a different keep-set. Heisenberg pares the state, which a re-weight leaves + // alone, so it follows exactly. + bool pared_from_operator{false}; }; /// A partition facade's snapshot: one child plan per partition, replayed together. @@ -128,9 +136,11 @@ class FunctionalPlan { })[0]; } const auto &local = std::get(shape_); - return fn(EvalRequest{.e_core = local.core_term, + // Held for the whole call: `weights` is what keeps the vector `request.op` refers to alive. + const auto weights = resolve_weights(local); + return fn(EvalRequest{.e_core = weights->core_term, .state = local.state, - .op = local.op, + .op = weights->op, .parameter_mapping = local.parameter_mapping, .gen_coeffs = local.gen_coeffs, .graph = local.graph->replay_view(), @@ -140,6 +150,26 @@ class FunctionalPlan { } private: + // The weights this call evaluates against: the propagator's live set, which a re-weight replaces + // between two calls. One load, so `op` and `core_term` cannot come from two publications. + // + // A functional is therefore a live view of the weights, not a frozen number: the same parameters give + // the new answer after a re-weight. Everything else a re-weight cannot leave intact -- a store row it + // would have to add, a graph it would have to re-pare -- either throws in + // MPOperator::update_initial_operator or is caught by validate_weight_refresh. + auto resolve_weights(const Local &local) const -> std::shared_ptr { + auto published = control_->weights.load(); + // Null cannot happen -- make_plan_ publishes -- and identity means no re-weight since this plan + // was built. Either way `local.weights` is the current set, so there is nothing to check. + if (published == nullptr || published == local.weights) { + return local.weights; + } + validate_weight_refresh({.weights_revision = published->structure_revision, + .expected_revision = expected_revision_, + .pared_from_operator = local.pared_from_operator}); + return published; + } + // Read straight off the borrowed operator, never through inverted_index(): that accessor rebuilds a // stale index, which is a write, and a plan must not write to its propagator. static auto operator_layout_unchanged(const Local &local) -> bool { @@ -160,6 +190,11 @@ class FunctionalPlan { /// Built by MonomialPropagator::expectation_value_functional(). It borrows from the propagator that /// made it (see detail::FunctionalPlan), so it must not outlive it, and a structural change to the /// propagator makes a call throw rather than answer. +/// +/// A re-weight is not a structural change: the functional follows the propagator's current +/// initial-operator weights, so two calls with the same parameters give two answers across a +/// MonomialPropagator::update_initial_operator(). Build the functional again after the last re-weight +/// to freeze a value. template class ExpectationValueFunctional { public: @@ -184,7 +219,8 @@ class ExpectationValueFunctional { }; /// As ExpectationValueFunctional, plus the gradient from the same backward pass: -/// `fn(parameters) -> (value, gradient)`, the gradient in parameter-axis order. +/// `fn(parameters) -> (value, gradient)`, the gradient in parameter-axis order. It follows the +/// initial-operator weights on the same terms. template class ExpectationValueAndGradientFunctional { public: diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index 82c61668..74a6d566 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -329,7 +329,7 @@ class MonomialPropagator { // Record that what a plan replays has moved. `site` must be a string literal: plans read it after // this propagator is gone. A mutation that is rejected outright must not bump -- it changed nothing, - // so it must not invalidate anything. A mutation that can fail part-way bumps before it starts + // so it must not invalidate anything. A mutation that can fail part-way bumps on the failure path // instead: invalidating a functional that did not need it costs a rebuild, answering from a snapshot // of a half-written operator costs a wrong number. auto bump_structure_(const char *site) -> void { @@ -337,6 +337,17 @@ class MonomialPropagator { functional_control_->structure_revision.fetch_add(1); } + // Hand the current initial-operator weights to every functional over this propagator, as one + // immutable set stamped with the revision it belongs to. Called on a re-weight, which is what makes a + // functional follow it, and never on a facade -- a facade holds no terms, and its partitions publish + // their own on their own masters. + auto publish_weights_() -> std::shared_ptr; + + // The weight set a new plan is built over: the published one when it still belongs to this revision, + // a fresh publication otherwise. Reusing matters -- a plan detects a re-weight by comparing pointers, + // so republishing an identical set would read as one. + auto weights_for_plan_() -> std::shared_ptr; + // A facade's own graph_/mp_op_ are never populated, so handing them out would return plausible-looking // empty state; there is no meaningful merge either, since the callers want one partition's raw layout. auto require_single_partition_(const char *what) const -> void { diff --git a/cpp/monoprop/Validation.cpp b/cpp/monoprop/Validation.cpp index 55113573..3b4fd0d0 100644 --- a/cpp/monoprop/Validation.cpp +++ b/cpp/monoprop/Validation.cpp @@ -122,6 +122,28 @@ auto validate_functional_state(const FunctionalState &state) -> void { } } +auto validate_weight_refresh(const WeightRefresh &refresh) -> void { + // A pared plan holds the layers pare_graph kept, and Schrodinger thresholds that keep-set from the + // operator coefficients themselves -- so new coefficients would need a different keep-set, and + // replaying this one would silently answer for a paring nobody asked for. Heisenberg thresholds the + // state, which a re-weight does not touch, so it follows exactly. + if (refresh.pared_from_operator) { + throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " + "initial operator was re-weighted, and this functional pares its graph " + "against the operator coefficients (a Schrodinger picture functional " + "with a pare_threshold), so it cannot follow the new weights. Create a " + "new functional."); + } + // Unreachable while every structural mutation bumps and every publication carries the revision it + // was made at: a functional whose revision still matches the propagator's cannot see weights from + // another revision. Kept as the backstop for the day one of those two stops being true. + if (refresh.weights_revision != refresh.expected_revision) { + throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " + "initial-operator weights it reads belong to a different graph. Create " + "a new functional."); + } +} + auto validate_only_rotate_len_k_(std::optional only_rotate_len_k, size_t max_k) -> void { if (!only_rotate_len_k.has_value()) { return; diff --git a/cpp/monoprop/Validation.h b/cpp/monoprop/Validation.h index 5f75ae0c..c84b07a5 100644 --- a/cpp/monoprop/Validation.h +++ b/cpp/monoprop/Validation.h @@ -52,6 +52,15 @@ struct FunctionalState { monoprop_EXPORT auto validate_functional_state(const FunctionalState &state) -> void; +/// The inputs to the check that a functional may follow a newer set of initial-operator weights. +struct WeightRefresh { + size_t weights_revision; ///< the structure revision the newer weights were published at + size_t expected_revision; ///< the revision the functional was built at + bool pared_from_operator; ///< the functional's keep-set was thresholded from the operator itself +}; + +monoprop_EXPORT auto validate_weight_refresh(const WeightRefresh &refresh) -> void; + // only_rotate_len_k is optional; when set it must satisfy 0 < k <= max_k. monoprop_EXPORT auto validate_only_rotate_len_k_(std::optional only_rotate_len_k, size_t max_k) -> void; diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index 885da5ff..825c5143 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -16,9 +16,25 @@ #include #include +#include + +#include "monoprop/TypeAliases.h" namespace monoprop::detail { +// The initial-operator weights an evaluation runs against, published as one immutable set. +// +// A re-weight writes only these two fields: MPOperator::update_initial_operator cannot add a store row, +// and apply_initial_operator_ writes core_term_ beside it, so the store, the inverted index and the graph +// all stay put. That is what lets a functional follow a re-weight instead of going stale. A published set +// is never edited, so one atomic load gives a caller a pair that belong together -- `op` and `core_term` +// read separately could come from two re-weights. +struct OperatorWeights { + VecD op; // un-evolved operator coefficients, one per store row + double core_term{0.0}; // the identity term, added to the summed expectation value + size_t structure_revision{0}; // the FunctionalControl revision these weights were published at +}; + // The validity block a propagator shares with every functional plan it makes. // // A plan borrows from its propagator, so before it reads any of those handles it needs two facts its @@ -41,6 +57,14 @@ struct FunctionalControl { // The method that last bumped structure_revision, so the error can name it. Always a string // literal, whose lifetime outlives every propagator. std::atomic last_structural_change{nullptr}; + + // The live initial-operator weights, republished by every re-weight and by the first plan built at a + // given revision. Null until the first plan is built: a propagator with no functionals has nobody to + // publish for, and publishing is a copy of `op`. + // + // Written only by the propagator, on its own thread (a facade publishes through for_each_partition_, + // so each partition publishes on its own pinned master). Read by a call through the plan. + std::atomic> weights{}; }; } // namespace monoprop::detail diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 969c34af..99a16697 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -355,31 +355,66 @@ auto MonomialPropagator::packed_inline_width_() const -> size_t { template auto MonomialPropagator::apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD> { - // Up front, not at the end: the distribution loop below can throw part-way (a rejected term, an - // out-of-range index) with core_term_ already written, and a facade can have some partitions applied. - bump_structure_("update_initial_operator()"); - if (partition_group_) { - // The facade holds no local terms of its own, so the return is empty. - for_each_partition_([&](MonomialPropagator &s) { s.update_initial_operator(op_dict); }); - return {}; - } - const size_t num_ranks = static_cast(mpi::size(comm_)); - const size_t my_rank = static_cast(mpi::rank(comm_)); - - OperatorDict new_op; - for (const auto &[ind, coeff] : op_dict) { - const auto mono = indices_to_bitset_checked(ind, 2 * logical_num_modes_); - if (ind.empty()) { // Core term, store in all - core_term_ = algebra_encode_coeff(basis_, coeff, mono); - continue; + try { + if (partition_group_) { + // The facade holds no local terms of its own, so the return is empty. Each partition publishes + // its own weights on its own master, which is what its children's functionals read. + for_each_partition_([&](MonomialPropagator &s) { s.update_initial_operator(op_dict); }); + return {}; } - if (my_rank == find_rank(mono, num_ranks)) { - const auto mono_indices = bitset_to_indices(mono); - new_op[mono_indices] = coeff; + const size_t num_ranks = static_cast(mpi::size(comm_)); + const size_t my_rank = static_cast(mpi::rank(comm_)); + + OperatorDict new_op; + for (const auto &[ind, coeff] : op_dict) { + const auto mono = indices_to_bitset_checked(ind, 2 * logical_num_modes_); + if (ind.empty()) { // Core term, store in all + core_term_ = algebra_encode_coeff(basis_, coeff, mono); + continue; + } + if (my_rank == find_rank(mono, num_ranks)) { + const auto mono_indices = bitset_to_indices(mono); + new_op[mono_indices] = coeff; + } } + + // No revision bump: a re-weight leaves the store, the inverted index and the graph where they are, + // so a live functional follows the new coefficients instead of going stale. Publication comes after + // the commit, so a functional never sees half of one. + auto applied = mp_op_.update_initial_operator(new_op, schrodinger_); + publish_weights_(); + return applied; + } + catch (...) { + // A part-applied re-weight is not one a functional may follow: the loop above can throw with + // core_term_ already written, and a facade can have applied some partitions. Nothing was published, + // so the bump is what stops a functional answering from weights the propagator disagrees with. + bump_structure_("update_initial_operator()"); + throw; } +} - return mp_op_.update_initial_operator(new_op, schrodinger_); +template +auto MonomialPropagator::publish_weights_() -> std::shared_ptr { + // get_operator() merges the pending init_op_map terms and so is a write: legal here, on the + // propagator's own thread, and never from a plan. + auto weights = std::make_shared( + detail::OperatorWeights{.op = mp_op_.get_operator(), + .core_term = core_term_, + .structure_revision = functional_control_->structure_revision.load()}); + functional_control_->weights.store(weights); + return weights; +} + +template +auto MonomialPropagator::weights_for_plan_() -> std::shared_ptr { + // A publication stamped with the current revision is still current: every mutation that can move the + // coefficients bumps, and a re-weight republishes. + if (auto published = functional_control_->weights.load(); + published != nullptr && published->structure_revision == functional_control_->structure_revision.load()) { + return published; + } + return publish_weights_(); } template @@ -963,8 +998,7 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho const auto sparse = mp_op_.sparse_state(); return EvalState::sparse(num_terms, sparse.rows, sparse.values); }(); - local.op = mp_op_.get_operator(); - local.core_term = this->core_term(); + local.weights = weights_for_plan_(); local.comm = comm_; const auto &inverted_index = mp_op_.inverted_index(); @@ -972,6 +1006,10 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.op_store = mp_op_.store.get(); local.inverted_index_rows = inverted_index.rows(); + // Only this combination pares against the coefficients a re-weight replaces, so only it must refuse to + // follow one. + local.pared_from_operator = schrodinger_ && pare_threshold.has_value(); + // The plan always owns its layers, so `cos` -- which holds a raw CosMask pointer per layer -- points // into layers no later append_layer, slice_graph or maybe_compact_layers can move or free. The copy is // cheap: a Layer is a shared_ptr to an immutable core plus an optional CosMask, and MPGraph::append @@ -985,9 +1023,9 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho return detail::fold_to_cos_mask(combined); }; // Threshold the picture's driving vector: the Hamiltonian in Schrödinger, the state otherwise. - const auto keep = - schrodinger_ ? indices_above(local.op, *pare_threshold) : local.state.indices_above(*pare_threshold); - const auto count = schrodinger_ ? local.op.size() : local.state.length(); + const auto keep = schrodinger_ ? indices_above(local.weights->op, *pare_threshold) + : local.state.indices_above(*pare_threshold); + const auto count = schrodinger_ ? local.weights->op.size() : local.state.length(); local.graph = std::make_shared(pare_graph(graph_, keep, count, schrodinger_, comm_, full_cos_of_layer)); } diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 3f6cca8e..47a92966 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -14,8 +14,7 @@ // What each public mutator does to a functional that was built before it ran. The table is the // contract: one row per mutating method, asserted for the value and the gradient functional, with -// and without a pare threshold, in both pictures. A cell that says Answers and carries a defect note -// records behaviour that is wrong today and is fixed by the stage the note names. +// and without a pare threshold, in both pictures. #include @@ -68,6 +67,27 @@ auto make_propagator(bool schrodinger, size_t partitions = 1) -> Prop { partitions); } +// The same propagator, built with the coefficients mutate_update_initial_operator() writes. A re-weight +// must leave a functional answering exactly what this propagator's answers. +auto make_reweighted_propagator(bool schrodinger, size_t partitions = 1) -> Prop { + OperatorDict initial_ham; + initial_ham[VecZ{0, 1}] = std::complex{0.0, 2.75}; + initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; + const auto cutoff = static_cast(2 * kNumModes); + return Prop(initial_ham, + cutoff, + VecZ{0, 1}, + schrodinger ? std::optional{cutoff} : std::nullopt, + MPI_COMM_SELF, + std::nullopt, + std::nullopt, + CutoffType::Support, + std::nullopt, + kNumModes, + Basis::Majorana, + partitions); +} + auto build_base_graph(Prop &prop) -> void { prop.build_graph(kBaseGates, VecZ{0, 1}, VecD{1.0, 1.0}); } @@ -122,16 +142,19 @@ auto mutate_update_upper_atol(Prop &prop) -> void { // What calling the pre-built functional does after the row's mutator ran. enum class Outcome : std::uint8_t { - Stale, // throws, reporting the propagator moved under the functional - Answers, // returns, and returns exactly what it returned before the mutation + Stale, // throws, reporting the propagator moved under the functional + Answers, // returns, and returns exactly what it returned before the mutation + Refreshes, // returns, and now returns what a functional built after the mutation returns + RefusesRefresh, // throws, reporting weights it cannot follow rather than a moved structure }; struct MutatorRow { std::string_view method; // the public method this row covers void (*apply)(Prop &); // bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted - Outcome exact; // pare_threshold == nullopt - Outcome pared; // pare_threshold == kPareThreshold + Outcome exact; // pare_threshold == nullopt, in either picture + Outcome pared; // pare_threshold == kPareThreshold, Heisenberg + Outcome pared_schrodinger; // pare_threshold == kPareThreshold, Schrodinger: pares from `op` std::string_view rationale; // }; @@ -141,6 +164,7 @@ constexpr std::array kMutatorTable{ .needs_empty_graph = false, .exact = Outcome::Stale, .pared = Outcome::Stale, + .pared_schrodinger = Outcome::Stale, .rationale = "Appending a layer moves the structure revision, which a pared plan reads as " "readily as an exact one."}, MutatorRow{.method = "propagate", @@ -148,6 +172,7 @@ constexpr std::array kMutatorTable{ .needs_empty_graph = true, .exact = Outcome::Stale, .pared = Outcome::Stale, + .pared_schrodinger = Outcome::Stale, .rationale = "Re-evolves the operator in place. It leaves the layer count at zero, which is " "why a layer count was never enough to see it."}, MutatorRow{.method = "contract_partially", @@ -155,20 +180,24 @@ constexpr std::array kMutatorTable{ .needs_empty_graph = false, .exact = Outcome::Stale, .pared = Outcome::Stale, + .pared_schrodinger = Outcome::Stale, .rationale = "Consumes the folded layers and rewrites the coefficients. Only inplace=true " "bumps; see contract_partially_out_of_place_keeps_functional_valid."}, MutatorRow{.method = "update_initial_operator", .apply = &mutate_update_initial_operator, .needs_empty_graph = false, - .exact = Outcome::Stale, - .pared = Outcome::Stale, - .rationale = "A re-weight bumps the revision. Stage 4 turns this into a weight refresh, " - "except for a pared Schrodinger plan."}, + .exact = Outcome::Refreshes, + .pared = Outcome::Refreshes, + .pared_schrodinger = Outcome::RefusesRefresh, + .rationale = "A re-weight moves no structure, so the functional follows the new " + "coefficients -- unless its keep-set was thresholded from those very " + "coefficients, which is Schrodinger with a pare threshold."}, MutatorRow{.method = "set_parameter_mapping", .apply = &mutate_set_parameter_mapping, .needs_empty_graph = false, .exact = Outcome::Stale, .pared = Outcome::Stale, + .pared_schrodinger = Outcome::Stale, .rationale = "Relabels the layers in place, which changes neither the layer count nor the " "operator -- the revision is the only thing that sees it."}, MutatorRow{.method = "update_cutoff", @@ -176,30 +205,35 @@ constexpr std::array kMutatorTable{ .needs_empty_graph = false, .exact = Outcome::Answers, .pared = Outcome::Answers, + .pared_schrodinger = Outcome::Answers, .rationale = "Intended: a cutoff gates the next build and changes nothing the plan holds."}, MutatorRow{.method = "update_cutoff_type", .apply = &mutate_update_cutoff_type, .needs_empty_graph = false, .exact = Outcome::Answers, .pared = Outcome::Answers, + .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_basis_change", .apply = &mutate_update_basis_change, .needs_empty_graph = false, .exact = Outcome::Answers, .pared = Outcome::Answers, + .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_lower_atol", .apply = &mutate_update_lower_atol, .needs_empty_graph = false, .exact = Outcome::Answers, .pared = Outcome::Answers, + .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_upper_atol", .apply = &mutate_update_upper_atol, .needs_empty_graph = false, .exact = Outcome::Answers, .pared = Outcome::Answers, + .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, }; @@ -232,6 +266,13 @@ auto make_call(Prop &prop, bool gradient, std::optional pare_threshold) return [fn = std::move(fn)](const VecD ¶ms) { return fn(params); }; } +auto expected_outcome(const MutatorRow &row, bool schrodinger, std::optional pare_threshold) -> Outcome { + if (!pare_threshold.has_value()) { + return row.exact; + } + return schrodinger ? row.pared_schrodinger : row.pared; +} + auto run_row(const MutatorRow &row, bool schrodinger, bool gradient, std::optional pare_threshold) -> void { auto prop = make_propagator(schrodinger); if (!row.needs_empty_graph) { @@ -244,18 +285,36 @@ auto run_row(const MutatorRow &row, bool schrodinger, bool gradient, std::option row.apply(prop); - const auto expected = pare_threshold.has_value() ? row.pared : row.exact; - if (expected == Outcome::Stale) { - BOOST_CHECK_EXCEPTION(call(params), std::runtime_error, [](const std::runtime_error &e) { - BOOST_TEST_INFO("message: " << e.what()); - return std::string_view(e.what()).find("MP object has been modified") != std::string_view::npos; - }); - return; + switch (expected_outcome(row, schrodinger, pare_threshold)) { + case Outcome::Stale: + BOOST_CHECK_EXCEPTION(call(params), std::runtime_error, [](const std::runtime_error &e) { + BOOST_TEST_INFO("message: " << e.what()); + return std::string_view(e.what()).find("MP object has been modified") != std::string_view::npos; + }); + return; + case Outcome::RefusesRefresh: + BOOST_CHECK_EXCEPTION(call(params), std::runtime_error, [](const std::runtime_error &e) { + BOOST_TEST_INFO("message: " << e.what()); + return std::string_view(e.what()).find("cannot follow the new weights") != std::string_view::npos; + }); + return; + case Outcome::Answers: { + // The plan replays its own snapshot, so the number cannot have moved. + double after = 0.0; + BOOST_REQUIRE_NO_THROW(after = call(params)); + BOOST_TEST(after == before, tt::tolerance(1e-12)); + return; + } + case Outcome::Refreshes: { + // The plan reads the propagator's live weights, so it must now agree exactly with a functional + // built after the mutation -- and disagree with what it answered before it. + double after = 0.0; + BOOST_REQUIRE_NO_THROW(after = call(params)); + BOOST_TEST(after == make_call(prop, gradient, pare_threshold)(params)); + BOOST_TEST(after != before); + return; + } } - // Answers: the plan replays its own snapshot, so the number cannot have moved. - double after = 0.0; - BOOST_REQUIRE_NO_THROW(after = call(params)); - BOOST_TEST(after == before, tt::tolerance(1e-12)); } auto run_table(bool schrodinger, std::optional pare_threshold) -> void { @@ -396,3 +455,112 @@ BOOST_AUTO_TEST_CASE(fanned_out_functional_is_invalidated_by_build_graph) { return std::string_view(e.what()).find("build_graph()") != std::string_view::npos; }); } + +// The refresh, end to end and at full precision: a re-weighted propagator's functional must answer what +// a propagator built with those coefficients answers, to the last bit. The two run the same arithmetic +// over the same store order, so anything less than equality means the refresh reached a different +// vector -- there is no rounding to hide behind here. +namespace { + +auto check_refresh_matches_fresh_propagator(bool gradient, std::optional pare_threshold, size_t partitions) + -> void { + auto reweighted = make_propagator(/*schrodinger=*/false, partitions); + build_base_graph(reweighted); + auto call = make_call(reweighted, gradient, pare_threshold); + const double before = call(kBaseParams); + mutate_update_initial_operator(reweighted); + + auto fresh = make_reweighted_propagator(/*schrodinger=*/false, partitions); + build_base_graph(fresh); + + BOOST_TEST(call(kBaseParams) == make_call(fresh, gradient, pare_threshold)(kBaseParams)); + BOOST_TEST(call(kBaseParams) != before); +} + +} // namespace + +BOOST_AUTO_TEST_CASE(reweighted_functional_matches_a_fresh_propagator) { + for (const bool gradient : {false, true}) { + for (const auto pare_threshold : {std::optional{}, std::optional{kPareThreshold}}) { + for (const size_t partitions : {1U, 2U}) { + BOOST_TEST_CONTEXT("gradient=" << gradient << " pared=" << pare_threshold.has_value() + << " partitions=" << partitions) { + check_refresh_matches_fresh_propagator(gradient, pare_threshold, partitions); + } + } + } + } +} + +// The gradient follows the weights too, component by component: a re-weight scales what the backward +// pass carries, so a value-only check would pass on a gradient that stayed behind. +BOOST_AUTO_TEST_CASE(reweighted_gradient_matches_a_fresh_propagator) { + auto reweighted = make_propagator(/*schrodinger=*/false); + build_base_graph(reweighted); + auto fn = reweighted.expectation_value_and_gradient_functional(std::nullopt); + const auto before = fn(kBaseParams); + mutate_update_initial_operator(reweighted); + + auto fresh = make_reweighted_propagator(/*schrodinger=*/false); + build_base_graph(fresh); + const auto expected = fresh.expectation_value_and_gradient_functional(std::nullopt)(kBaseParams); + + const auto after = fn(kBaseParams); + BOOST_TEST(after.second == expected.second, tt::per_element()); + BOOST_CHECK(after.second != before.second); +} + +// Invariant 4: Schrodinger thresholds its keep-set from the operator coefficients, so new coefficients +// select a different keep-set and the plan cannot replay this one. It says so rather than answering for a +// paring nobody asked for. Heisenberg thresholds the state, which a re-weight leaves alone. +BOOST_AUTO_TEST_CASE(pared_schrodinger_functional_refuses_to_follow_a_reweight) { + auto prop = make_propagator(/*schrodinger=*/true); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, kPareThreshold); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + + mutate_update_initial_operator(prop); + + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, [](const std::runtime_error &e) { + const std::string_view what(e.what()); + BOOST_TEST_INFO("message: " << what); + return what.find("pares its graph") != std::string_view::npos + && what.find("cannot follow the new weights") != std::string_view::npos; + }); + // The unpared plan over the same propagator follows the re-weight, so the refusal is the paring's and + // not the picture's. + BOOST_CHECK_NO_THROW(make_call(prop, /*gradient=*/false, std::nullopt)(kBaseParams)); +} + +// A rejected re-weight is not a refresh: MPOperator::update_initial_operator throws for a term the +// operator does not hold, and core_term_ may already carry the new value by then. A functional must +// report that rather than answer from weights the propagator disagrees with. +BOOST_AUTO_TEST_CASE(a_failed_reweight_invalidates_the_functional) { + auto prop = make_propagator(/*schrodinger=*/false); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + + OperatorDict unknown_term; + unknown_term[VecZ{0, 2}] = std::complex{0.0, 1.0}; + BOOST_CHECK_THROW(prop.update_initial_operator(unknown_term), std::runtime_error); + + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, [](const std::runtime_error &e) { + return std::string_view(e.what()).find("update_initial_operator()") != std::string_view::npos; + }); +} + +// Building a second functional must not look like a re-weight to the first: both are built over the one +// published weight set, so the pared Schrodinger plan -- the only one that refuses to follow a new set -- +// keeps answering. +BOOST_AUTO_TEST_CASE(building_another_functional_is_not_a_reweight) { + auto prop = make_propagator(/*schrodinger=*/true); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, kPareThreshold); + const double before = call(kBaseParams); + + auto other = make_call(prop, /*gradient=*/true, std::nullopt); + BOOST_CHECK_NO_THROW(other(kBaseParams)); + + BOOST_TEST(call(kBaseParams) == before); +} diff --git a/cpp/tests/update_initial_operator.cpp b/cpp/tests/update_initial_operator.cpp index 76dc74ac..850ebe3d 100644 --- a/cpp/tests/update_initial_operator.cpp +++ b/cpp/tests/update_initial_operator.cpp @@ -53,12 +53,12 @@ BOOST_AUTO_TEST_CASE(update_initial_operator_updates_core_expval) { auto updated_fn = simulator.expectation_value_functional(std::nullopt); BOOST_TEST(updated_fn(empty_params) == 2.75, tt::tolerance(1e-12)); - // The functional built before the re-weight snapshotted the old coefficients, so it must reject - // the call rather than answer for an operator the propagator no longer holds. - BOOST_CHECK_THROW(expval_fn(empty_params), std::runtime_error); + // A re-weight moves no structure, so the functional built before it follows the new coefficients: + // the same call now gives the new core term, not the old one it was built over. + BOOST_TEST(expval_fn(empty_params) == 2.75, tt::tolerance(1e-12)); } -BOOST_AUTO_TEST_CASE(update_initial_operator_invalidates_gradient_functional) { +BOOST_AUTO_TEST_CASE(update_initial_operator_refreshes_gradient_functional) { constexpr size_t n_modes = 2; OperatorDict initial_ham; initial_ham[VecZ{}] = std::complex{1.0, 0.0}; @@ -82,7 +82,8 @@ BOOST_AUTO_TEST_CASE(update_initial_operator_invalidates_gradient_functional) { updated[VecZ{}] = std::complex{2.75, 0.0}; simulator.update_initial_operator(updated); - BOOST_CHECK_THROW(grad_fn(empty_params), std::runtime_error); + // As for the value functional: the gradient functional follows the weights rather than going stale. + BOOST_TEST(grad_fn(empty_params).first == 2.75, tt::tolerance(1e-12)); BOOST_TEST(simulator.expectation_value_and_gradient_functional(std::nullopt)(empty_params).first == 2.75, tt::tolerance(1e-12)); } diff --git a/cpp/tests/validation_tests.cpp b/cpp/tests/validation_tests.cpp index 3f784bca..24b0aff9 100644 --- a/cpp/tests/validation_tests.cpp +++ b/cpp/tests/validation_tests.cpp @@ -83,6 +83,24 @@ BOOST_AUTO_TEST_CASE(validation_functional_state) { BOOST_CHECK_THROW(validate_functional_state(rebuilt), std::runtime_error); } +BOOST_AUTO_TEST_CASE(validation_weight_refresh) { + const WeightRefresh followable{.weights_revision = 3, .expected_revision = 3, .pared_from_operator = false}; + BOOST_CHECK_NO_THROW(validate_weight_refresh(followable)); + + // The keep-set came from the coefficients the re-weight replaced, so replaying it would answer for a + // paring nobody asked for. + auto pared_from_op = followable; + pared_from_op.pared_from_operator = true; + BOOST_CHECK_EXCEPTION(validate_weight_refresh(pared_from_op), std::runtime_error, [](const auto &e) { + return std::string_view(e.what()).find("cannot follow the new weights") != std::string_view::npos; + }); + + // The backstop: weights from another revision reached a functional whose own revision still matches. + auto other_revision = followable; + other_revision.weights_revision = 2; + BOOST_CHECK_THROW(validate_weight_refresh(other_revision), std::runtime_error); +} + BOOST_AUTO_TEST_CASE(validation_only_rotate_len_k) { BOOST_CHECK_NO_THROW(validate_only_rotate_len_k_(std::nullopt, 8)); BOOST_CHECK_NO_THROW(validate_only_rotate_len_k_(8u, 8)); diff --git a/src/monoprop/majorana_propagator.py b/src/monoprop/majorana_propagator.py index 477b70fd..0c4b491f 100644 --- a/src/monoprop/majorana_propagator.py +++ b/src/monoprop/majorana_propagator.py @@ -128,8 +128,9 @@ def update_initial_operator(self, new_operator: MajoranaOperator) -> None: Re-weights the initial operator the graph is evaluated against, without touching the evolution graph or rebuilding the simulator. Only the initial operator is affected -- the gates and their generator coefficients are unchanged. Functionals created - earlier are invalidated; see - [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator]. + earlier follow the new coefficients rather than being invalidated; see + [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] + for the exception and the price. Args: new_operator: A [MajoranaOperator][monoprop.majorana.MajoranaOperator] whose terms diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index 63be1c0b..3242999c 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -382,9 +382,15 @@ def expectation_value_functional( ) -> Callable[..., float]: """Return a reusable callable computing the expectation value from parameters. - The callable is built against the graph and initial-operator coefficients present now, so - mutating either -- [build_graph][], [contract_partially][], [update_initial_operator][] -- - invalidates it; build a new one after such a call. + The callable is built against the graph present now, so a structural change to it -- + [build_graph][], [propagate][], [contract_partially][] with ``inplace=True``, or a new + [parameter_mapping][] -- invalidates the callable; build a new one after such a call. + + A re-weight is not a structural change: the callable follows the propagator's current + initial-operator coefficients, so an [update_initial_operator][] leaves it valid and it + answers for the new weights. The price is that it is a live view of those weights rather + than a frozen number -- two calls with the same parameters give two answers across a + re-weight. Build the callable again after the last re-weight to freeze a value. Args: pare_threshold: Edge-retention cutoff for this functional's masked plan: edges @@ -395,8 +401,11 @@ def expectation_value_functional( A callable ``fn(parameters=None) -> float``. Raises: - RuntimeError: From the returned callable, if the propagator was mutated after this - functional was created. + RuntimeError: From the returned callable, if the propagator was structurally mutated + after this functional was created, or if it was re-weighted and this functional + cannot follow the new weights -- which is the Schrodinger picture with a + ``pare_threshold``, whose pared graph was selected from the very coefficients the + re-weight replaced. """ fn = self._simulator.expectation_value_functional(pare_threshold) return lambda parameters=None: fn(self._bind(parameters)) @@ -407,7 +416,8 @@ def expectation_value_and_gradient_functional( """Return a reusable callable computing (expectation value, gradient). Like [expectation_value_functional][], but one backward pass also yields the gradient. It is - invalidated by the same mutations. + invalidated by the same mutations, and follows the initial-operator weights on the same + terms: value and gradient both answer for the current coefficients. Args: pare_threshold: See [expectation_value_functional][]. @@ -416,8 +426,8 @@ def expectation_value_and_gradient_functional( A callable ``fn(parameters=None) -> (float, np.ndarray)``, gradient in parameter order. Raises: - RuntimeError: From the returned callable, if the propagator was mutated after this - functional was created. + RuntimeError: From the returned callable, on the same conditions as + [expectation_value_functional][]. """ fn = self._simulator.expectation_value_and_gradient_functional(pare_threshold) @@ -541,8 +551,15 @@ def update_initial_operator(self, new_operator: T_op) -> None: Each concrete front-end implements this over its own operator type, encoding the terms into the engine's raw index tuples. - Functionals hold the coefficients they were built with, so any created earlier are - invalidated -- they raise instead of answering for the replaced operator. + Functionals created earlier stay valid and follow the new coefficients: a re-weight moves no + structure, so there is nothing for them to be stale about. They are therefore a live view of + the weights -- two calls with the same parameters give two answers across this call. The one + exception is a functional built in the Schrodinger picture with a ``pare_threshold``: its + pared graph was selected from the coefficients this call replaces, so it raises instead of + following them. + + A rejected re-weight invalidates them, since this call can fail with the core term already + written: they raise rather than answer for weights the propagator disagrees with. Args: new_operator: A [MajoranaOperator][monoprop.majorana.MajoranaOperator] or diff --git a/src/monoprop/pauli_propagator.py b/src/monoprop/pauli_propagator.py index 1b0ec4e8..de026c11 100644 --- a/src/monoprop/pauli_propagator.py +++ b/src/monoprop/pauli_propagator.py @@ -221,8 +221,9 @@ def update_initial_operator(self, new_operator: PauliOperator) -> None: base method, which takes the engine's raw symplectic-slot keys, this accepts qubit Pauli terms and encodes them via [get_local_operator][monoprop.pauli.PauliOperator.get_local_operator]. Functionals created - earlier are invalidated; see - [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator]. + earlier follow the new coefficients rather than being invalidated; see + [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] + for the exception and the price. Args: new_operator: A [PauliOperator][monoprop.pauli.PauliOperator] whose terms replace the diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index dc0b7f06..5d9b9aff 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -169,7 +169,7 @@ def test_functional_invalidated_after_graph_mutation(self, serial_comm): "expectation_value_and_gradient_functional", ], ) - def test_functional_invalidated_after_initial_operator_update( + def test_functional_follows_initial_operator_update( self, serial_comm, propagator_cls, @@ -192,18 +192,21 @@ def test_functional_invalidated_after_initial_operator_update( ) functional = getattr(mp, functional_name)() parameters = [0.3, 0.7] - functional(parameters) - mp.update_initial_operator(updated_operator) + def call(fn): + result = fn(parameters) + return result[0] if isinstance(result, tuple) else result - # The functional snapshotted the old coefficients, so it must reject the call rather than - # keep answering for the operator the propagator no longer holds. - with pytest.raises(RuntimeError, match=r"MP object has been modified"): - functional(parameters) + before = call(functional) - rebuilt = getattr(mp, functional_name)()(parameters) - rebuilt_expval = rebuilt[0] if isinstance(rebuilt, tuple) else rebuilt - assert rebuilt_expval == pytest.approx(mp.expval(parameters)) + mp.update_initial_operator(updated_operator) + + # A re-weight moves no structure, so the functional built before it follows the new + # coefficients instead of refusing the call: it now answers what the propagator answers. + after = call(functional) + assert after != pytest.approx(before) + assert after == pytest.approx(mp.expval(parameters)) + assert after == call(getattr(mp, functional_name)()) class TestFunctionalValidityTable: @@ -234,9 +237,11 @@ def _base_circuit(cls): ) @classmethod - def _propagator(cls, comm, *, schrodinger, with_graph): + def _propagator(cls, comm, *, schrodinger, with_graph, first_weight=1.0): mp = MajoranaPropagator( - MajoranaOperator({(0, 1): 1.0j, (2, 3): 0.5j}, num_modes=cls._MODES), + MajoranaOperator( + {(0, 1): first_weight * 1j, (2, 3): 0.5j}, num_modes=cls._MODES + ), [0, 1], cutoff=cls._CUTOFF, schrodinger_cutoff=cls._CUTOFF if schrodinger else None, @@ -290,8 +295,10 @@ def _mutate_lower_atol(mp): def _mutate_upper_atol(mp): mp.upper_atol = 1e-3 - # (method, mutator, needs_empty_graph, exact, pared, rationale). "stale" = the call must throw, - # "answers" = it must return exactly what it returned before the mutation. + # (method, mutator, needs_empty_graph, exact, pared, pared_schrodinger, rationale). + # "stale" = the call must throw that the propagator moved, "answers" = it must return exactly what + # it returned before the mutation, "refreshes" = it must return what a functional built after the + # mutation returns, "refuses-refresh" = it must throw that it cannot follow the new weights. ROWS = ( ( "build_graph", @@ -299,6 +306,7 @@ def _mutate_upper_atol(mp): False, "stale", "stale", + "stale", "Appending a layer moves the structure revision, which a pared plan reads as readily as " "an exact one.", ), @@ -308,6 +316,7 @@ def _mutate_upper_atol(mp): True, "stale", "stale", + "stale", "Re-evolves the operator in place. It leaves the layer count at zero, which is why a " "layer count was never enough to see it.", ), @@ -317,16 +326,19 @@ def _mutate_upper_atol(mp): False, "stale", "stale", + "stale", "Consumes the folded layers and rewrites the coefficients. Only inplace=True bumps.", ), ( "update_initial_operator", "_mutate_update_initial_operator", False, - "stale", - "stale", - "A re-weight bumps the revision. Stage 4 turns this into a weight refresh, except for " - "a pared Schrodinger plan.", + "refreshes", + "refreshes", + "refuses-refresh", + "A re-weight moves no structure, so the functional follows the new coefficients -- " + "unless its keep-set was thresholded from those very coefficients, which is " + "Schrodinger with a pare threshold.", ), ( "parameter_mapping", @@ -334,6 +346,7 @@ def _mutate_upper_atol(mp): False, "stale", "stale", + "stale", "Relabels the layers in place, which changes neither the layer count nor the operator -- " "the revision is the only thing that sees it.", ), @@ -343,6 +356,7 @@ def _mutate_upper_atol(mp): False, "answers", "answers", + "answers", "Intended: a cutoff gates the next build and changes nothing the plan holds.", ), ( @@ -351,6 +365,7 @@ def _mutate_upper_atol(mp): False, "answers", "answers", + "answers", "Intended: as cutoff.", ), ( @@ -359,6 +374,7 @@ def _mutate_upper_atol(mp): False, "answers", "answers", + "answers", "Intended: as cutoff.", ), ( @@ -367,6 +383,7 @@ def _mutate_upper_atol(mp): False, "answers", "answers", + "answers", "Intended: as cutoff.", ), ( @@ -375,6 +392,7 @@ def _mutate_upper_atol(mp): False, "answers", "answers", + "answers", "Intended: as cutoff.", ), ) @@ -416,7 +434,15 @@ def test_mutator_effect_on_live_functional( partitions, row, ): - method, mutator, needs_empty_graph, exact, pared_outcome, rationale = row + ( + method, + mutator, + needs_empty_graph, + exact, + pared_outcome, + pared_schrodinger, + rationale, + ) = row monkeypatch.setenv("monoprop_PARTITIONS", partitions) mp = self._propagator( @@ -426,19 +452,29 @@ def test_mutator_effect_on_live_functional( threshold = self._PARE_THRESHOLD if pared else None functional = getattr(mp, functional_name)(threshold) - def call(): - result = functional(parameters) + def call(fn=None): + result = (fn or functional)(parameters) return result[0] if isinstance(result, tuple) else result before = call() getattr(self, mutator)(mp) - expected = pared_outcome if pared else exact + expected = exact + if pared: + expected = pared_schrodinger if schrodinger else pared_outcome + context = f"{method}: {rationale}" if expected == "stale": with pytest.raises(RuntimeError, match=r"MP object has been modified"): call() + elif expected == "refuses-refresh": + with pytest.raises(RuntimeError, match=r"cannot follow the new weights"): + call() + elif expected == "refreshes": + after = call() + assert after == call(getattr(mp, functional_name)(threshold)), context + assert after != pytest.approx(before), context else: - assert call() == pytest.approx(before), f"{method}: {rationale}" + assert call() == pytest.approx(before), context @pytest.mark.parametrize("partitions", ["off", "auto"]) @pytest.mark.parametrize( @@ -475,6 +511,65 @@ def test_parameter_mapping_invalidates_functional_it_desynchronises( with pytest.raises(RuntimeError, match=r"set_parameter_mapping"): functional(parameters) + @pytest.mark.parametrize("partitions", ["off", "auto"]) + @pytest.mark.parametrize("pared", [False, True], ids=["exact", "pared"]) + @pytest.mark.parametrize( + "functional_name", + [ + "expectation_value_functional", + "expectation_value_and_gradient_functional", + ], + ) + def test_reweighted_functional_matches_a_fresh_propagator( + self, monkeypatch, serial_comm, functional_name, pared, partitions + ): + """The refresh at full precision: a re-weighted propagator's functional must answer what a + propagator built with those coefficients answers, to the last bit. Both run the same + arithmetic over the same store order, so there is no rounding to hide behind. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + threshold = self._PARE_THRESHOLD if pared else None + parameters = list(self._PARAMS) + + def value(result): + return result[0] if isinstance(result, tuple) else result + + mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) + functional = getattr(mp, functional_name)(threshold) + before = value(functional(parameters)) + self._mutate_update_initial_operator(mp) + + fresh = self._propagator( + serial_comm, schrodinger=False, with_graph=True, first_weight=2.75 + ) + expected = value(getattr(fresh, functional_name)(threshold)(parameters)) + + after = value(functional(parameters)) + assert after == expected + assert after != pytest.approx(before) + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_pared_schrodinger_functional_refuses_to_follow_a_reweight( + self, monkeypatch, serial_comm, partitions + ): + """Schrodinger thresholds its keep-set from the operator coefficients, so new coefficients + select a different keep-set: the plan says so rather than replaying a paring nobody asked + for. The unpared functional over the same propagator follows the re-weight, which is what + makes the refusal the paring's and not the picture's. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=True, with_graph=True) + parameters = list(self._PARAMS) + pared = mp.expectation_value_functional(self._PARE_THRESHOLD) + exact = mp.expectation_value_functional() + pared(parameters) + + self._mutate_update_initial_operator(mp) + + with pytest.raises(RuntimeError, match=r"cannot follow the new weights"): + pared(parameters) + assert exact(parameters) == pytest.approx(mp.expval(parameters)) + @pytest.mark.parametrize("partitions", ["off", "auto"]) def test_pared_functional_is_invalidated_by_build_graph( self, monkeypatch, serial_comm, partitions From 0dd7bbb04662c41b2e1354c7facd8b99de43dea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 19 Aug 2026 14:16:26 +0000 Subject: [PATCH 07/14] docs(propagator): :memo: state the weight-refresh contract, and let a functional 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 --- AGENTS.md | 12 +++++- cpp/include/monoprop/Functional.h | 18 +++++++++ cpp/tests/functional_validity.cpp | 17 ++++++++ cspell.json | 2 + docs/content/docs/features/evaluation.mdx | 39 ++++++++++++++----- docs/content/docs/features/initialisation.mdx | 4 ++ src/monoprop/bindings/binder.h | 10 ++++- src/monoprop/monomial_propagator.py | 18 +++++++-- tests/test_parameter_validation.py | 25 ++++++++++++ 9 files changed, 128 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 27af1c40..0ba6f596 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -78,7 +78,11 @@ Key files: single-partition shape and the facade shape, so both paths have one public type, and the value and the gradient functional over one snapshot share one plan. A functional borrows from its propagator (the inverted index always, the graph unless pared), so it must not outlive it; the bindings pin that with - `nb::keep_alive<0, 1>`. + `nb::keep_alive<0, 1>`. It does **not** snapshot the initial-operator weights: it reads the + `detail::OperatorWeights` set the propagator has published, so it follows an + `update_initial_operator` instead of going stale — the one exception being a Schrödinger plan with a + `pare_threshold`, whose keep-set came from the coefficients the re-weight replaced, which throws + (`follows_weights` reports which case an object is). - `src/monoprop/bindings/binder.h`: hand-written binding template; `tools/generate-*.py` generate the per-mode-width `bindings.cpp` and `_dispatch.py` from it (do not hand-edit the generated files). Both generators take the 32-mode storage-block rule from `tools/_binding_layout.py` — they must @@ -110,7 +114,11 @@ Key files: rejected mutation must not bump); the settings that only gate the next build — the atols, the cutoff, the cutoff type, the basis change — deliberately do not. A plan additionally re-derives the operator's store pointer and inverted-index row count as a backstop, so a missing bump reports staleness instead - of folding a rebuilt index. + of folding a rebuilt index. The block also carries the published `OperatorWeights`: a re-weight + publishes a new set rather than bumping, which is what lets a live functional follow it, and bumps only + if it fails part-way. Publishing runs on the propagator's own thread (a facade publishes through + `for_each_partition_`), and a plan reads the set once per call so `op` and `core_term` cannot come from + two publications. - **The partition facade**: `partitions > 1` makes a `MonomialPropagator` a facade over S single-partition propagators, one hash partition each. Every method that fans out must use the private partition vocabulary declared in `MonomialPropagator.h` (`for_each_partition_`, `map_partitions_`, `concat_partitions_` diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index 53353135..52a6edc7 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -104,6 +104,16 @@ class FunctionalPlan { /// The parameter-axis length the plan was built against; a call must supply exactly this many. auto num_params() const -> size_t { return num_params_; } + /// Whether a call after a re-weight answers for the new coefficients, rather than throwing. + auto follows_weights() const -> bool { + if (const auto *fanout = std::get_if(&shape_)) { + // Every child was built in the same picture at the same threshold, so one child answers for + // all of them. Reading an immutable child field needs no fan-out. + return fanout->partitions.empty() || fanout->partitions.front()->follows_weights(); + } + return !std::get(shape_).pared_from_operator; + } + /// Throw unless `params` fits and the propagator still holds what the plan replays. // A facade checks its own control block here -- the group it fans out over belongs to the facade -- // and each child plan then checks its own partition's, on that partition's master thread. Only the @@ -201,6 +211,10 @@ class ExpectationValueFunctional { /// The parameter-axis length this functional was built against. auto num_params() const -> size_t { return plan_->num_params(); } + /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as + /// this object holds it, so a caller need not re-derive it from picture and pare threshold. + auto follows_weights() const -> bool { return plan_->follows_weights(); } + auto operator()(const VecD ¶meters) const -> double { return plan_->evaluate( [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> double { @@ -227,6 +241,10 @@ class ExpectationValueAndGradientFunctional { /// The parameter-axis length this functional was built against. auto num_params() const -> size_t { return plan_->num_params(); } + /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as + /// this object holds it, so a caller need not re-derive it from picture and pare threshold. + auto follows_weights() const -> bool { return plan_->follows_weights(); } + auto operator()(const VecD ¶meters) const -> std::pair { return plan_->evaluate( [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> std::pair { diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 47a92966..6d547fbe 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -426,6 +426,23 @@ BOOST_AUTO_TEST_CASE(functional_reports_its_parameter_axis) { BOOST_TEST(prop.expectation_value_functional(kPareThreshold).num_params() == kBaseParams.size()); } +// The contract read off the object, for callers that hold a functional and not the propagator: only a +// Schrodinger plan pared against the operator's own coefficients refuses to follow a re-weight. +BOOST_AUTO_TEST_CASE(functional_reports_whether_it_follows_the_weights) { + for (const bool schrodinger : {false, true}) { + auto prop = make_propagator(schrodinger); + build_base_graph(prop); + BOOST_TEST(prop.expectation_value_functional().follows_weights()); + BOOST_TEST(prop.expectation_value_and_gradient_functional().follows_weights()); + BOOST_TEST(prop.expectation_value_functional(kPareThreshold).follows_weights() == !schrodinger); + } + // A facade answers for its partitions, which were all built the same way. + auto facade = make_propagator(/*schrodinger=*/true, /*partitions=*/2); + build_base_graph(facade); + BOOST_TEST(facade.expectation_value_functional().follows_weights()); + BOOST_TEST(!facade.expectation_value_functional(kPareThreshold).follows_weights()); +} + // A facade's plan holds the partition group by raw pointer, so it needs the facade's own control block: // the children cannot report a destruction that takes their group with it. BOOST_AUTO_TEST_CASE(fanned_out_functional_reports_a_destroyed_facade) { diff --git a/cspell.json b/cspell.json index 4f86409b..9de770db 100644 --- a/cspell.json +++ b/cspell.json @@ -52,6 +52,8 @@ "qubit", "qubits", "Remigio", + "reweight", + "reweighted", "tracemalloc", "schrodinger", "simulable", diff --git a/docs/content/docs/features/evaluation.mdx b/docs/content/docs/features/evaluation.mdx index 75943bcc..957c93a5 100644 --- a/docs/content/docs/features/evaluation.mdx +++ b/docs/content/docs/features/evaluation.mdx @@ -47,15 +47,36 @@ expval, grad = expval_grad_fn(parameters) Shorter aliases are also available: `sim.expval_functional()` and `sim.expval_and_grad_functional()`. -A functional is built against the graph and initial-operator coefficients present when it is -created, so mutating either — -[build_graph][monoprop.monomial_propagator.MonomialPropagator.build_graph], -[contract_partially][monoprop.monomial_propagator.MonomialPropagator.contract_partially], -[update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] — -invalidates it: calling it afterwards raises `RuntimeError` rather than returning a value for state -the propagator no longer holds. Build a new functional after such a call. The direct -[expectation_value][monoprop.monomial_propagator.MonomialPropagator.expectation_value] path always -reflects the current operator. +A functional is built against the graph present when it is created, and it reads through to the +propagator it came from, which it keeps alive for as long as you hold it. A structural change to that +graph invalidates the functional: calling it afterwards raises `RuntimeError` rather than returning a +value for a circuit the propagator no longer holds. Build a new functional after such a call. The +direct [expectation_value][monoprop.monomial_propagator.MonomialPropagator.expectation_value] path +always reflects the current propagator. + +A re-weight is the one mutation a functional survives: +[update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] +leaves the graph, the operator's terms and their index where they are, so a live functional follows +the new coefficients. The price is that a functional is a *live view* of those coefficients — two +calls with the same parameters give two answers across a re-weight. Build the functional again after +the last re-weight if you need a frozen value. Each functional carries the rule as a +`follows_weights` attribute, so a caller holding one need not re-derive it from the picture and the +threshold. + +This is what every public mutating method does to a functional built before it ran: + +| Method | A call afterwards | +| --- | --- | +| [build_graph][monoprop.monomial_propagator.MonomialPropagator.build_graph] | raises | +| [propagate][monoprop.monomial_propagator.MonomialPropagator.propagate] | raises | +| [contract_partially][monoprop.monomial_propagator.MonomialPropagator.contract_partially] with `inplace=True` | raises | +| [parameter_mapping][monoprop.monomial_propagator.MonomialPropagator.parameter_mapping] | raises | +| [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] | answers for the new coefficients — except in the Schrödinger picture with a `pare_threshold`, where it raises, because the pared graph was selected from the coefficients the re-weight replaced | +| a rejected `update_initial_operator` | raises: the call can fail with the core term already written | +| `cutoff`, `cutoff_type`, `basis_change`, `lower_atol`, `upper_atol` | answers, unchanged: these gate the next build and touch nothing a functional replays | +| `contract_partially` with `inplace=False` | answers, unchanged: it mutates nothing | + +The table is executable — see [the mutation table](/testing#the-mutation-table). Both functionals accept an optional `pare_threshold` — see *Paring* below. diff --git a/docs/content/docs/features/initialisation.mdx b/docs/content/docs/features/initialisation.mdx index 95dc79ab..06698097 100644 --- a/docs/content/docs/features/initialisation.mdx +++ b/docs/content/docs/features/initialisation.mdx @@ -44,6 +44,10 @@ Schrödinger picture the state is evolved independently, so you can change the observable arbitrarily and read it off against the same evolved state; in the Heisenberg picture the observable can be re-weighted. +Functionals built before the update stay valid and answer for the new coefficients, so a re-weight +loop does not have to rebuild them — see [Reusable functionals](/features/evaluation#reusable-functionals) +for the one exception and for what a live view of the weights costs. + Only terms that already exist in the initial operator can be updated; supplying a monomial that is not present (or a non-Hermitian coefficient) raises diff --git a/src/monoprop/bindings/binder.h b/src/monoprop/bindings/binder.h index 87740aa5..ac9b5c17 100644 --- a/src/monoprop/bindings/binder.h +++ b/src/monoprop/bindings/binder.h @@ -67,7 +67,10 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { "Expectation value at the given variational parameters") .def_prop_ro("num_params", &ExpectationValueFunctional::num_params, - "Parameter-axis length this functional was built against"); + "Parameter-axis length this functional was built against") + .def_prop_ro("follows_weights", + &ExpectationValueFunctional::follows_weights, + "Whether a call after update_initial_operator() answers for the new weights"); auto grad_name = std::format("ExpectationValueAndGradientFunctional{:03d}", NumModes); nb::class_>(mod, grad_name.c_str()) @@ -77,7 +80,10 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { "(expectation value, gradient) at the given variational parameters") .def_prop_ro("num_params", &ExpectationValueAndGradientFunctional::num_params, - "Parameter-axis length this functional was built against"); + "Parameter-axis length this functional was built against") + .def_prop_ro("follows_weights", + &ExpectationValueAndGradientFunctional::follows_weights, + "Whether a call after update_initial_operator() answers for the new weights"); cls.def( "__init__", diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index 3242999c..552b8564 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -398,7 +398,8 @@ def expectation_value_functional( accuracy for speed. ``None`` (default) disables paring. Returns: - A callable ``fn(parameters=None) -> float``. + A callable ``fn(parameters=None) -> float``, carrying the rule above as a + ``follows_weights`` attribute. Raises: RuntimeError: From the returned callable, if the propagator was structurally mutated @@ -408,7 +409,14 @@ def expectation_value_functional( re-weight replaced. """ fn = self._simulator.expectation_value_functional(pare_threshold) - return lambda parameters=None: fn(self._bind(parameters)) + + def _call(parameters: ParameterValues = None) -> float: + return fn(self._bind(parameters)) + + # Carried through to the caller's object rather than left on the engine functional, which is + # not part of the public surface. + _call.follows_weights = fn.follows_weights # type: ignore[attr-defined] + return _call def expectation_value_and_gradient_functional( self, pare_threshold: float | None = None @@ -423,7 +431,8 @@ def expectation_value_and_gradient_functional( pare_threshold: See [expectation_value_functional][]. Returns: - A callable ``fn(parameters=None) -> (float, np.ndarray)``, gradient in parameter order. + A callable ``fn(parameters=None) -> (float, np.ndarray)``, gradient in parameter order, + carrying a ``follows_weights`` attribute as [expectation_value_functional][] does. Raises: RuntimeError: From the returned callable, on the same conditions as @@ -431,10 +440,11 @@ def expectation_value_and_gradient_functional( """ fn = self._simulator.expectation_value_and_gradient_functional(pare_threshold) - def _call(parameters=None): # noqa: ANN001, ANN202 + def _call(parameters: ParameterValues = None) -> tuple[float, np.ndarray]: value, grad = fn(self._bind(parameters)) return value, np.asarray(grad, dtype=np.float64) + _call.follows_weights = fn.follows_weights # type: ignore[attr-defined] return _call def expval( diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index 5d9b9aff..0ad02af8 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -511,6 +511,31 @@ def test_parameter_mapping_invalidates_functional_it_desynchronises( with pytest.raises(RuntimeError, match=r"set_parameter_mapping"): functional(parameters) + @pytest.mark.parametrize("partitions", ["off", "auto"]) + @pytest.mark.parametrize( + "factory", + [ + "expectation_value_functional", + "expectation_value_and_gradient_functional", + ], + ) + def test_bound_functional_reports_whether_it_follows_weights( + self, monkeypatch, serial_comm, factory, partitions + ): + """The contract read off the object: only a Schrodinger plan pared against the operator's own + coefficients refuses to follow a re-weight. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + for schrodinger in (False, True): + mp = self._propagator(serial_comm, schrodinger=schrodinger, with_graph=True) + for threshold in (None, self._PARE_THRESHOLD): + follows = not (schrodinger and threshold is not None) + engine = getattr(mp._simulator, factory)(threshold) + assert engine.follows_weights is follows + # The front end wraps the engine functional in a callable, which has to carry the + # rule through: the engine object is not part of the public surface. + assert getattr(mp, factory)(threshold).follows_weights is follows + @pytest.mark.parametrize("partitions", ["off", "auto"]) @pytest.mark.parametrize("pared", [False, True], ids=["exact", "pared"]) @pytest.mark.parametrize( From 6b2dd2e7d27cda1e846f2b4ced195ec574bc1700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 07:25:40 +0000 Subject: [PATCH 08/14] refactor(propagator): :recycle: one definition each for the weight rule, the 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 --- cpp/include/monoprop/Functional.h | 2 +- cpp/monoprop/Validation.cpp | 11 +-- cpp/monoprop/Validation.h | 2 +- .../MonomialPropagator.inl | 19 ++-- cpp/tests/functional_validity.cpp | 60 +++++-------- cpp/tests/validation_tests.cpp | 12 +-- src/monoprop/monomial_propagator.py | 61 ++++++++----- tests/test_parameter_validation.py | 90 +++++++++---------- 8 files changed, 129 insertions(+), 128 deletions(-) diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index 52a6edc7..6078c617 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -176,7 +176,7 @@ class FunctionalPlan { } validate_weight_refresh({.weights_revision = published->structure_revision, .expected_revision = expected_revision_, - .pared_from_operator = local.pared_from_operator}); + .may_follow_weights = follows_weights()}); return published; } diff --git a/cpp/monoprop/Validation.cpp b/cpp/monoprop/Validation.cpp index 3b4fd0d0..a476b908 100644 --- a/cpp/monoprop/Validation.cpp +++ b/cpp/monoprop/Validation.cpp @@ -123,11 +123,12 @@ auto validate_functional_state(const FunctionalState &state) -> void { } auto validate_weight_refresh(const WeightRefresh &refresh) -> void { - // A pared plan holds the layers pare_graph kept, and Schrodinger thresholds that keep-set from the - // operator coefficients themselves -- so new coefficients would need a different keep-set, and - // replaying this one would silently answer for a paring nobody asked for. Heisenberg thresholds the - // state, which a re-weight does not touch, so it follows exactly. - if (refresh.pared_from_operator) { + // The caller passes its own follows_weights(), the property it advertises, so what is enforced here + // and what is advertised cannot drift apart. It is false for exactly one shape: a pared Schrodinger + // plan holds a keep-set thresholded from the operator coefficients themselves, so new coefficients + // would need a different keep-set and replaying this one would answer for a paring nobody asked for. + // Heisenberg thresholds the state, which a re-weight does not touch, so it follows exactly. + if (!refresh.may_follow_weights) { throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " "initial operator was re-weighted, and this functional pares its graph " "against the operator coefficients (a Schrodinger picture functional " diff --git a/cpp/monoprop/Validation.h b/cpp/monoprop/Validation.h index c84b07a5..7bd046a7 100644 --- a/cpp/monoprop/Validation.h +++ b/cpp/monoprop/Validation.h @@ -56,7 +56,7 @@ monoprop_EXPORT auto validate_functional_state(const FunctionalState &state) -> struct WeightRefresh { size_t weights_revision; ///< the structure revision the newer weights were published at size_t expected_revision; ///< the revision the functional was built at - bool pared_from_operator; ///< the functional's keep-set was thresholded from the operator itself + bool may_follow_weights; ///< the functional's own follows_weights(), so the rule has one definition }; monoprop_EXPORT auto validate_weight_refresh(const WeightRefresh &refresh) -> void; diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 99a16697..a33a0571 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -382,7 +382,13 @@ auto MonomialPropagator::apply_initial_operator_(const OperatorDict &o // so a live functional follows the new coefficients instead of going stale. Publication comes after // the commit, so a functional never sees half of one. auto applied = mp_op_.update_initial_operator(new_op, schrodinger_); - publish_weights_(); + // Only refresh a set somebody has already been given: a null one means no plan was ever built, + // and weights_for_plan_() publishes on demand, so there is nothing left stale by skipping this. + // Publishing copies the whole coefficient vector, which a re-weight loop would otherwise pay per + // call on a propagator with no functionals. + if (functional_control_->weights.load() != nullptr) { + publish_weights_(); + } return applied; } catch (...) { @@ -973,11 +979,10 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho typename Plan::Fanout fanout; fanout.group = partition_group_.get(); fanout.partitions = map_partitions_([&](MonomialPropagator &s) { return s.make_plan_(pare_threshold); }); - // graph_gate_arrays_() reads partition 0: the graph structure and gate info are identical on - // every partition. A facade validates nothing itself (see FunctionalPlan::validate). - return std::make_shared(expected_num_params(graph_gate_arrays_().first), - functional_control_, - std::move(fanout)); + // The children hold the same graph structure and gate info, so one of them already carries the + // parameter-axis length. A facade validates nothing else itself (see FunctionalPlan::validate). + const auto num_params = fanout.partitions.front()->num_params(); + return std::make_shared(num_params, functional_control_, std::move(fanout)); } typename Plan::Local local; @@ -1030,6 +1035,8 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho std::make_shared(pare_graph(graph_, keep, count, schrodinger_, comm_, full_cos_of_layer)); } else { + // Rebuilt from the active layers rather than copied off graph_: MPGraph keeps what a slice + // retired in front of front_offset_, and a plan has no use for those layers. std::vector owned; owned.reserve(graph_.layers()); for (size_t i = 0; i < graph_.layers(); ++i) { diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 6d547fbe..db2b68b0 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -47,31 +47,14 @@ const VecD kBaseParams{0.3, 0.7}; // symmetric under swapping the two angles -- which is what makes the set_parameter_mapping row bite. const std::vector kBaseGates{VecZ{0}, VecZ{2}}; -// `partitions` is passed explicitly, so it wins over the suite-wide monoprop_PARTITIONS=off. -auto make_propagator(bool schrodinger, size_t partitions = 1) -> Prop { - OperatorDict initial_ham; - initial_ham[VecZ{0, 1}] = std::complex{0.0, 1.0}; - initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; - const auto cutoff = static_cast(2 * kNumModes); - return Prop(initial_ham, - cutoff, - VecZ{0, 1}, - schrodinger ? std::optional{cutoff} : std::nullopt, - MPI_COMM_SELF, - std::nullopt, - std::nullopt, - CutoffType::Support, - std::nullopt, - kNumModes, - Basis::Majorana, - partitions); -} +// The weight mutate_update_initial_operator() writes onto term (0, 1). A re-weighted propagator must +// answer exactly like one built with it from the start, so both sides read it from here. +constexpr double kReweightedFirstWeight = 2.75; -// The same propagator, built with the coefficients mutate_update_initial_operator() writes. A re-weight -// must leave a functional answering exactly what this propagator's answers. -auto make_reweighted_propagator(bool schrodinger, size_t partitions = 1) -> Prop { +// `partitions` is passed explicitly, so it wins over the suite-wide monoprop_PARTITIONS=off. +auto make_propagator(bool schrodinger, size_t partitions = 1, double first_weight = 1.0) -> Prop { OperatorDict initial_ham; - initial_ham[VecZ{0, 1}] = std::complex{0.0, 2.75}; + initial_ham[VecZ{0, 1}] = std::complex{0.0, first_weight}; initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; const auto cutoff = static_cast(2 * kNumModes); return Prop(initial_ham, @@ -110,7 +93,7 @@ auto mutate_contract_partially(Prop &prop) -> void { auto mutate_update_initial_operator(Prop &prop) -> void { OperatorDict updated; - updated[VecZ{0, 1}] = std::complex{0.0, 2.75}; + updated[VecZ{0, 1}] = std::complex{0.0, kReweightedFirstWeight}; updated[VecZ{2, 3}] = std::complex{0.0, 0.5}; prop.update_initial_operator(updated); } @@ -149,13 +132,13 @@ enum class Outcome : std::uint8_t { }; struct MutatorRow { - std::string_view method; // the public method this row covers - void (*apply)(Prop &); // - bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted - Outcome exact; // pare_threshold == nullopt, in either picture - Outcome pared; // pare_threshold == kPareThreshold, Heisenberg - Outcome pared_schrodinger; // pare_threshold == kPareThreshold, Schrodinger: pares from `op` - std::string_view rationale; // + std::string_view method; // the public method this row covers + void (*apply)(Prop &); + bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted + Outcome exact; // pare_threshold == nullopt, in either picture + Outcome pared; // pare_threshold == kPareThreshold, Heisenberg + Outcome pared_schrodinger; // pare_threshold == kPareThreshold, Schrodinger: pares from `op` + std::string_view rationale; }; constexpr std::array kMutatorTable{ @@ -173,8 +156,8 @@ constexpr std::array kMutatorTable{ .exact = Outcome::Stale, .pared = Outcome::Stale, .pared_schrodinger = Outcome::Stale, - .rationale = "Re-evolves the operator in place. It leaves the layer count at zero, which is " - "why a layer count was never enough to see it."}, + .rationale = "Re-evolves the operator in place. It leaves the layer count at zero, so the " + "revision is the only thing that sees it."}, MutatorRow{.method = "contract_partially", .apply = &mutate_contract_partially, .needs_empty_graph = false, @@ -252,7 +235,7 @@ constexpr auto distinct_methods() -> size_t { return distinct; } -static_assert(distinct_methods() == Prop::num_mutating_methods, +static_assert(kMutatorTable.size() == Prop::num_mutating_methods && distinct_methods() == kMutatorTable.size(), "cpp/tests/functional_validity.cpp must carry one row per public mutating method of " "MonomialPropagator; see num_mutating_methods in MonomialPropagator.h."); @@ -371,9 +354,8 @@ BOOST_AUTO_TEST_CASE(propagate_on_non_empty_graph_leaves_functional_valid) { BOOST_TEST(call(kBaseParams) == before, tt::tolerance(1e-12)); } -// The two rows those tests cover used to answer instead of throwing, each returning a number for a -// circuit nobody had asked about any more. These say so directly: the propagator's own answer moves, -// and the functional refuses rather than following it half way. +// Two of the table's Stale rows, spelled out: the propagator's own answer moves, and the functional +// refuses the call rather than following it half way. BOOST_AUTO_TEST_CASE(set_parameter_mapping_invalidates_functional_it_desynchronises) { auto prop = make_propagator(/*schrodinger=*/false); @@ -487,7 +469,7 @@ auto check_refresh_matches_fresh_propagator(bool gradient, std::optional const double before = call(kBaseParams); mutate_update_initial_operator(reweighted); - auto fresh = make_reweighted_propagator(/*schrodinger=*/false, partitions); + auto fresh = make_propagator(/*schrodinger=*/false, partitions, kReweightedFirstWeight); build_base_graph(fresh); BOOST_TEST(call(kBaseParams) == make_call(fresh, gradient, pare_threshold)(kBaseParams)); @@ -518,7 +500,7 @@ BOOST_AUTO_TEST_CASE(reweighted_gradient_matches_a_fresh_propagator) { const auto before = fn(kBaseParams); mutate_update_initial_operator(reweighted); - auto fresh = make_reweighted_propagator(/*schrodinger=*/false); + auto fresh = make_propagator(/*schrodinger=*/false, /*partitions=*/1, kReweightedFirstWeight); build_base_graph(fresh); const auto expected = fresh.expectation_value_and_gradient_functional(std::nullopt)(kBaseParams); diff --git a/cpp/tests/validation_tests.cpp b/cpp/tests/validation_tests.cpp index 24b0aff9..4de8a96d 100644 --- a/cpp/tests/validation_tests.cpp +++ b/cpp/tests/validation_tests.cpp @@ -84,14 +84,14 @@ BOOST_AUTO_TEST_CASE(validation_functional_state) { } BOOST_AUTO_TEST_CASE(validation_weight_refresh) { - const WeightRefresh followable{.weights_revision = 3, .expected_revision = 3, .pared_from_operator = false}; + const WeightRefresh followable{.weights_revision = 3, .expected_revision = 3, .may_follow_weights = true}; BOOST_CHECK_NO_THROW(validate_weight_refresh(followable)); - // The keep-set came from the coefficients the re-weight replaced, so replaying it would answer for a - // paring nobody asked for. - auto pared_from_op = followable; - pared_from_op.pared_from_operator = true; - BOOST_CHECK_EXCEPTION(validate_weight_refresh(pared_from_op), std::runtime_error, [](const auto &e) { + // What a pared Schrodinger functional reports: its keep-set came from the coefficients the re-weight + // replaced, so replaying it would answer for a paring nobody asked for. + auto refuses = followable; + refuses.may_follow_weights = false; + BOOST_CHECK_EXCEPTION(validate_weight_refresh(refuses), std::runtime_error, [](const auto &e) { return std::string_view(e.what()).find("cannot follow the new weights") != std::string_view::npos; }); diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index 552b8564..0313d8a2 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -55,6 +55,37 @@ T_op = TypeVar("T_op", MajoranaOperator, PauliOperator) +class _BoundFunctional: + """One engine functional, called through the front end's parameter binding. + + A call has to resolve named or circuit-carried parameters first, so the caller cannot be handed + the engine functional itself. Forwarding everything else by attribute, rather than copying named + properties across, keeps this in step with the bindings on its own. + """ + + def __init__(self, propagator: MonomialPropagator, functional: object) -> None: + self._propagator = propagator + self._functional = functional + + def __call__(self, parameters: ParameterValues = None) -> float: + return self._functional(self._propagator._bind(parameters)) + + def __getattr__(self, name: str) -> object: + # Private names are answered by the instance dictionary alone: reaching the engine object for + # one would recurse here through `self._functional` before __init__ has set it. + if name.startswith("_"): + raise AttributeError(name) + return getattr(self._functional, name) + + +class _BoundGradientFunctional(_BoundFunctional): + """As the value functional, but returning ``(value, gradient)`` with the gradient as ``float64``.""" + + def __call__(self, parameters: ParameterValues = None) -> tuple[float, np.ndarray]: + value, grad = self._functional(self._propagator._bind(parameters)) + return value, np.asarray(grad, dtype=np.float64) + + class MonomialPropagator(ABC, Generic[T_op]): """Abstract base for the classical monomial-propagation simulators. @@ -398,8 +429,8 @@ def expectation_value_functional( accuracy for speed. ``None`` (default) disables paring. Returns: - A callable ``fn(parameters=None) -> float``, carrying the rule above as a - ``follows_weights`` attribute. + A callable ``fn(parameters=None) -> float``, exposing the rule above as + ``follows_weights`` and its parameter-axis length as ``num_params``. Raises: RuntimeError: From the returned callable, if the propagator was structurally mutated @@ -408,15 +439,9 @@ def expectation_value_functional( ``pare_threshold``, whose pared graph was selected from the very coefficients the re-weight replaced. """ - fn = self._simulator.expectation_value_functional(pare_threshold) - - def _call(parameters: ParameterValues = None) -> float: - return fn(self._bind(parameters)) - - # Carried through to the caller's object rather than left on the engine functional, which is - # not part of the public surface. - _call.follows_weights = fn.follows_weights # type: ignore[attr-defined] - return _call + return _BoundFunctional( + self, self._simulator.expectation_value_functional(pare_threshold) + ) def expectation_value_and_gradient_functional( self, pare_threshold: float | None = None @@ -432,20 +457,16 @@ def expectation_value_and_gradient_functional( Returns: A callable ``fn(parameters=None) -> (float, np.ndarray)``, gradient in parameter order, - carrying a ``follows_weights`` attribute as [expectation_value_functional][] does. + with the same ``follows_weights`` and ``num_params`` as [expectation_value_functional][]. Raises: RuntimeError: From the returned callable, on the same conditions as [expectation_value_functional][]. """ - fn = self._simulator.expectation_value_and_gradient_functional(pare_threshold) - - def _call(parameters: ParameterValues = None) -> tuple[float, np.ndarray]: - value, grad = fn(self._bind(parameters)) - return value, np.asarray(grad, dtype=np.float64) - - _call.follows_weights = fn.follows_weights # type: ignore[attr-defined] - return _call + return _BoundGradientFunctional( + self, + self._simulator.expectation_value_and_gradient_functional(pare_threshold), + ) def expval( self, diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index 0ad02af8..9b3dab08 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -29,6 +29,11 @@ from monoprop.pauli import PauliOperator +def _value(result): + """The value component of a functional's answer, for either functional kind.""" + return result[0] if isinstance(result, tuple) else result + + def _two_gate_graph(serial_comm): """A propagator with a two-layer, two-parameter graph already built.""" operator = MajoranaOperator({(0, 1): 1.0j, (2, 3): 0.5j}, num_modes=2) @@ -192,35 +197,33 @@ def test_functional_follows_initial_operator_update( ) functional = getattr(mp, functional_name)() parameters = [0.3, 0.7] - - def call(fn): - result = fn(parameters) - return result[0] if isinstance(result, tuple) else result - - before = call(functional) + before = _value(functional(parameters)) mp.update_initial_operator(updated_operator) # A re-weight moves no structure, so the functional built before it follows the new # coefficients instead of refusing the call: it now answers what the propagator answers. - after = call(functional) + after = _value(functional(parameters)) assert after != pytest.approx(before) assert after == pytest.approx(mp.expval(parameters)) - assert after == call(getattr(mp, functional_name)()) + assert after == _value(getattr(mp, functional_name)()(parameters)) class TestFunctionalValidityTable: """What each public mutator does to a functional built before it ran. The Python mirror of ``cpp/tests/functional_validity.cpp``: same rows, same expectations, run - over both partition settings. The build-time coverage gate lives on the C++ side - (``MonomialPropagator::num_mutating_methods``); here the roster is asserted as data. + over both partition settings. The gate that a new mutator gets a row is the C++ static_assert + against ``MonomialPropagator::num_mutating_methods``; these rows mirror that table. """ _MODES = 2 _CUTOFF = 4 _PARAMS = (0.3, 0.7) _PARE_THRESHOLD = 1e-12 + # What _mutate_update_initial_operator() writes onto term (0, 1); a re-weighted propagator must + # answer exactly like one built with it from the start, so both sides read it from here. + _REWEIGHTED_FIRST_WEIGHT = 2.75 # One gate per Hamiltonian term, and the terms carry different weights, so the answer is not # symmetric under swapping the two angles -- which is what makes the parameter_mapping row bite. @@ -267,7 +270,10 @@ def _mutate_contract_partially(cls, mp): @classmethod def _mutate_update_initial_operator(cls, mp): mp.update_initial_operator( - MajoranaOperator({(0, 1): 2.75j, (2, 3): 0.5j}, num_modes=cls._MODES) + MajoranaOperator( + {(0, 1): cls._REWEIGHTED_FIRST_WEIGHT * 1j, (2, 3): 0.5j}, + num_modes=cls._MODES, + ) ) @staticmethod @@ -317,8 +323,8 @@ def _mutate_upper_atol(mp): "stale", "stale", "stale", - "Re-evolves the operator in place. It leaves the layer count at zero, which is why a " - "layer count was never enough to see it.", + "Re-evolves the operator in place. It leaves the layer count at zero, so the revision " + "is the only thing that sees it.", ), ( "contract_partially", @@ -397,20 +403,6 @@ def _mutate_upper_atol(mp): ), ) - def test_table_covers_every_public_mutator(self): - assert {row[0] for row in self.ROWS} == { - "build_graph", - "propagate", - "contract_partially", - "update_initial_operator", - "parameter_mapping", - "cutoff", - "cutoff_type", - "basis_change", - "lower_atol", - "upper_atol", - } - @pytest.mark.parametrize("row", ROWS, ids=[row[0] for row in ROWS]) @pytest.mark.parametrize("partitions", ["off", "auto"]) @pytest.mark.parametrize("pared", [False, True], ids=["exact", "pared"]) @@ -452,11 +444,7 @@ def test_mutator_effect_on_live_functional( threshold = self._PARE_THRESHOLD if pared else None functional = getattr(mp, functional_name)(threshold) - def call(fn=None): - result = (fn or functional)(parameters) - return result[0] if isinstance(result, tuple) else result - - before = call() + before = _value(functional(parameters)) getattr(self, mutator)(mp) expected = exact @@ -465,16 +453,17 @@ def call(fn=None): context = f"{method}: {rationale}" if expected == "stale": with pytest.raises(RuntimeError, match=r"MP object has been modified"): - call() + functional(parameters) elif expected == "refuses-refresh": with pytest.raises(RuntimeError, match=r"cannot follow the new weights"): - call() + functional(parameters) elif expected == "refreshes": - after = call() - assert after == call(getattr(mp, functional_name)(threshold)), context + after = _value(functional(parameters)) + fresh = getattr(mp, functional_name)(threshold) + assert after == _value(fresh(parameters)), context assert after != pytest.approx(before), context else: - assert call() == pytest.approx(before), context + assert _value(functional(parameters)) == pytest.approx(before), context @pytest.mark.parametrize("partitions", ["off", "auto"]) @pytest.mark.parametrize( @@ -489,15 +478,16 @@ def test_bound_functional_reports_its_parameter_axis( ): monkeypatch.setenv("monoprop_PARTITIONS", partitions) mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) - functional = getattr(mp._simulator, factory)(None) - assert functional.num_params == len(self._PARAMS) + assert getattr(mp._simulator, factory)(None).num_params == len(self._PARAMS) + # The front end hands back its own callable, which forwards what the engine object exposes. + assert getattr(mp, factory)(None).num_params == len(self._PARAMS) @pytest.mark.parametrize("partitions", ["off", "auto"]) def test_parameter_mapping_invalidates_functional_it_desynchronises( self, monkeypatch, serial_comm, partitions ): - """The relabel moves the propagator's own answer, and the functional refuses rather than - following it half way -- it used to keep returning the pre-relabel number. + """The relabel moves the propagator's own answer, and the functional refuses the call rather + than following it half way. """ monkeypatch.setenv("monoprop_PARTITIONS", partitions) mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) @@ -532,8 +522,8 @@ def test_bound_functional_reports_whether_it_follows_weights( follows = not (schrodinger and threshold is not None) engine = getattr(mp._simulator, factory)(threshold) assert engine.follows_weights is follows - # The front end wraps the engine functional in a callable, which has to carry the - # rule through: the engine object is not part of the public surface. + # The engine object is not part of the public surface, so the rule has to be readable + # off what the front end returns. assert getattr(mp, factory)(threshold).follows_weights is follows @pytest.mark.parametrize("partitions", ["off", "auto"]) @@ -556,20 +546,20 @@ def test_reweighted_functional_matches_a_fresh_propagator( threshold = self._PARE_THRESHOLD if pared else None parameters = list(self._PARAMS) - def value(result): - return result[0] if isinstance(result, tuple) else result - mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) functional = getattr(mp, functional_name)(threshold) - before = value(functional(parameters)) + before = _value(functional(parameters)) self._mutate_update_initial_operator(mp) fresh = self._propagator( - serial_comm, schrodinger=False, with_graph=True, first_weight=2.75 + serial_comm, + schrodinger=False, + with_graph=True, + first_weight=self._REWEIGHTED_FIRST_WEIGHT, ) - expected = value(getattr(fresh, functional_name)(threshold)(parameters)) + expected = _value(getattr(fresh, functional_name)(threshold)(parameters)) - after = value(functional(parameters)) + after = _value(functional(parameters)) assert after == expected assert after != pytest.approx(before) From 15e571a0034abc52acb8e8419982031ad970310c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 14:05:58 +0200 Subject: [PATCH 09/14] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Roberto Di Remigio Eikås Signed-off-by: Roberto Di Remigio Eikås --- cpp/monoprop/Validation.cpp | 12 ------------ cpp/monoprop/Validation.h | 2 -- cpp/monoprop/detail/functional/Control.h | 7 ++----- 3 files changed, 2 insertions(+), 19 deletions(-) diff --git a/cpp/monoprop/Validation.cpp b/cpp/monoprop/Validation.cpp index a476b908..24f1be5d 100644 --- a/cpp/monoprop/Validation.cpp +++ b/cpp/monoprop/Validation.cpp @@ -100,8 +100,6 @@ auto validate_functional_call(const VecD ¶meters, size_t expected_num_params } auto validate_functional_state(const FunctionalState &state) -> void { - // Aliveness first: every other handle a functional holds points into the propagator, so there is - // nothing else it may legally read once this is false. if (!state.propagator_alive) { throw StaleFunctionalGraph("The propagator this functional was built from has been destroyed. " "A functional reads the propagator's operator index directly, so it " @@ -114,8 +112,6 @@ auto validate_functional_state(const FunctionalState &state) -> void { "graph or operator the functional replays. Create a new functional.", state.last_structural_change != nullptr ? state.last_structural_change : "a structural mutation")); } - // No bump, but the operator moved anyway: some mutation is missing its bump_structure_ call. Report - // it as staleness rather than read a rebuilt inverted index through a pointer to the old one. if (!state.operator_layout_unchanged) { throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " "operator index it reads was rebuilt or grown. Create a new functional."); @@ -123,11 +119,6 @@ auto validate_functional_state(const FunctionalState &state) -> void { } auto validate_weight_refresh(const WeightRefresh &refresh) -> void { - // The caller passes its own follows_weights(), the property it advertises, so what is enforced here - // and what is advertised cannot drift apart. It is false for exactly one shape: a pared Schrodinger - // plan holds a keep-set thresholded from the operator coefficients themselves, so new coefficients - // would need a different keep-set and replaying this one would answer for a paring nobody asked for. - // Heisenberg thresholds the state, which a re-weight does not touch, so it follows exactly. if (!refresh.may_follow_weights) { throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " "initial operator was re-weighted, and this functional pares its graph " @@ -135,9 +126,6 @@ auto validate_weight_refresh(const WeightRefresh &refresh) -> void { "with a pare_threshold), so it cannot follow the new weights. Create a " "new functional."); } - // Unreachable while every structural mutation bumps and every publication carries the revision it - // was made at: a functional whose revision still matches the propagator's cannot see weights from - // another revision. Kept as the backstop for the day one of those two stops being true. if (refresh.weights_revision != refresh.expected_revision) { throw StaleFunctionalGraph("MP object has been modified since the functional was created: the " "initial-operator weights it reads belong to a different graph. Create " diff --git a/cpp/monoprop/Validation.h b/cpp/monoprop/Validation.h index 7bd046a7..1d9146b0 100644 --- a/cpp/monoprop/Validation.h +++ b/cpp/monoprop/Validation.h @@ -40,8 +40,6 @@ monoprop_EXPORT auto validate_parameters_length(const VecD ¶ms, const VecZ & monoprop_EXPORT auto validate_functional_call(const VecD ¶meters, size_t expected_num_params) -> void; /// What a functional must be able to say about its propagator before it reads anything it borrowed. -// Assembled from detail::FunctionalControl plus, for a single-partition plan, two facts derived from the -// operator itself. The derived pair is the backstop: it holds even for a mutation that forgot to bump. struct FunctionalState { bool propagator_alive; ///< false once ~MonomialPropagator has run size_t current_revision; ///< the propagator's structure revision now diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index 825c5143..348c403e 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -37,11 +37,8 @@ struct OperatorWeights { // The validity block a propagator shares with every functional plan it makes. // -// A plan borrows from its propagator, so before it reads any of those handles it needs two facts its -// own snapshot cannot supply: whether the propagator is still there, and whether the structure the -// snapshot describes is still the propagator's. Both live here, behind one shared_ptr, so a plan -// answers them without dereferencing the propagator at all. The propagator holds the only mutating -// handle; plans hold shared_ptr. +// A plan borrows from its propagator, so before it reads any of those handles it needs two facts: whether the propagator is still there, and whether the structure the +// snapshot describes is still the propagator's. // // A copied propagator gets its own block: a copy carries no functionals, so it starts at revision 0. struct FunctionalControl { From bd26296dd0e45988b16b85072e7c78cb57a0fec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 12:02:37 +0000 Subject: [PATCH 10/14] chore: round with claude simplify --- AGENTS.md | 14 ++-- cpp/include/monoprop/Functional.h | 70 ++++++++----------- cpp/include/monoprop/MonomialPropagator.h | 4 +- .../MonomialPropagator.inl | 9 ++- cpp/tests/functional_validity.cpp | 42 ++++------- src/monoprop/bindings/binder.h | 51 +++++++------- src/monoprop/monomial_propagator.py | 29 ++++---- tests/test_parameter_validation.py | 21 ++---- 8 files changed, 105 insertions(+), 135 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0ba6f596..e6e90677 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -72,12 +72,14 @@ Key files: (the Majorana/Pauli choice is a runtime `Basis`, not a separate class). Its `only_rotate_len_k` arguments use `std::optional`; `std::nullopt` means no gate-application length cap. - `cpp/include/monoprop/Functional.h`: the two functional objects, - `ExpectationValueFunctional` and `ExpectationValueAndGradientFunctional`. Each is - a handle on one shared `detail::FunctionalPlan` — 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 value and the - gradient functional over one snapshot share one plan. A functional borrows from its propagator (the - inverted index always, the graph unless pared), so it must not outlive it; the bindings pin that with + `ExpectationValueFunctional` and `ExpectationValueAndGradientFunctional`. Both derive + from `detail::FunctionalHandle` — the shared handle half — and each holds a + `detail::FunctionalPlan`: 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 plan holds the snapshot rather than the + choice of what to compute, so one plan type backs either kind (each factory call builds its own). A + functional borrows from its propagator (the inverted index always, the graph unless pared), so it + must not outlive it; the bindings pin that with `nb::keep_alive<0, 1>`. It does **not** snapshot the initial-operator weights: it reads the `detail::OperatorWeights` set the propagator has published, so it follows an `update_initial_operator` instead of going stale — the one exception being a Schrödinger plan with a diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index 6078c617..dcef6d81 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -41,9 +41,9 @@ namespace detail { /// One propagator snapshot a functional replays, plus the checks that say the snapshot is still that /// propagator's own. /// -/// Immutable once built and held by `shared_ptr`, so the value and the gradient functional over -/// the same snapshot share one plan. Every field is either owned or, where the comment says so, -/// borrowed from the propagator — which is why a functional must not outlive it. +/// Immutable once built and held by `shared_ptr`, so one plan backs either functional kind: it +/// holds the snapshot, not the choice of what to compute. Every field is either owned or, where the +/// comment says so, borrowed from the propagator — which is why a functional must not outlive it. template class FunctionalPlan { public: @@ -109,7 +109,7 @@ class FunctionalPlan { if (const auto *fanout = std::get_if(&shape_)) { // Every child was built in the same picture at the same threshold, so one child answers for // all of them. Reading an immutable child field needs no fan-out. - return fanout->partitions.empty() || fanout->partitions.front()->follows_weights(); + return fanout->partitions.front()->follows_weights(); } return !std::get(shape_).pared_from_operator; } @@ -141,9 +141,9 @@ class FunctionalPlan { if (const auto *fanout = std::get_if(&shape_)) { // Each partition allreduces internally, so partition 0 already carries the global answer. // The fan-out must reach every master: the partitions' collectives are barrier-synced. - return partition::collect_on_all(*fanout->group, [&](int r) -> R { + return std::move(partition::collect_on_all(*fanout->group, [&](int r) -> R { return fanout->partitions[static_cast(r)]->evaluate(fn, params); - })[0]; + })[0]); } const auto &local = std::get(shape_); // Held for the whole call: `weights` is what keeps the vector `request.op` refers to alive. @@ -193,6 +193,24 @@ class FunctionalPlan { std::variant shape_; }; +/// The handle half both functional kinds share: the plan they replay, and the two facts a caller can +/// ask about it without calling it. The kinds differ only in what `operator()` computes. +template +class FunctionalHandle { +public: + /// The parameter-axis length this functional was built against. + auto num_params() const -> size_t { return plan_->num_params(); } + + /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as + /// this object holds it, so a caller need not re-derive it from picture and pare threshold. + auto follows_weights() const -> bool { return plan_->follows_weights(); } + +protected: + explicit FunctionalHandle(std::shared_ptr> plan) : plan_(std::move(plan)) {} + + std::shared_ptr> plan_; +}; + } // namespace detail /// A reusable expectation value over one propagator snapshot: `fn(parameters) -> double`. @@ -206,60 +224,32 @@ class FunctionalPlan { /// MonomialPropagator::update_initial_operator(). Build the functional again after the last re-weight /// to freeze a value. template -class ExpectationValueFunctional { +class ExpectationValueFunctional : public detail::FunctionalHandle { public: - /// The parameter-axis length this functional was built against. - auto num_params() const -> size_t { return plan_->num_params(); } - - /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as - /// this object holds it, so a caller need not re-derive it from picture and pare threshold. - auto follows_weights() const -> bool { return plan_->follows_weights(); } - - auto operator()(const VecD ¶meters) const -> double { - return plan_->evaluate( - [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> double { - return ev(request, comm, cos); - }, - parameters); - } + auto operator()(const VecD ¶meters) const -> double { return this->plan_->evaluate(ev, parameters); } private: friend class MonomialPropagator; explicit ExpectationValueFunctional(std::shared_ptr> plan) - : plan_(std::move(plan)) {} - - std::shared_ptr> plan_; + : detail::FunctionalHandle(std::move(plan)) {} }; /// As ExpectationValueFunctional, plus the gradient from the same backward pass: /// `fn(parameters) -> (value, gradient)`, the gradient in parameter-axis order. It follows the /// initial-operator weights on the same terms. template -class ExpectationValueAndGradientFunctional { +class ExpectationValueAndGradientFunctional : public detail::FunctionalHandle { public: - /// The parameter-axis length this functional was built against. - auto num_params() const -> size_t { return plan_->num_params(); } - - /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as - /// this object holds it, so a caller need not re-derive it from picture and pare threshold. - auto follows_weights() const -> bool { return plan_->follows_weights(); } - auto operator()(const VecD ¶meters) const -> std::pair { - return plan_->evaluate( - [](const EvalRequest &request, mpi::Comm comm, const detail::CosCallbacks &cos) -> std::pair { - return ev_and_grad(request, comm, cos); - }, - parameters); + return this->plan_->evaluate(ev_and_grad, parameters); } private: friend class MonomialPropagator; explicit ExpectationValueAndGradientFunctional(std::shared_ptr> plan) - : plan_(std::move(plan)) {} - - std::shared_ptr> plan_; + : detail::FunctionalHandle(std::move(plan)) {} }; } // namespace monoprop diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index 74a6d566..4bdc61d7 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -98,9 +98,7 @@ class MonomialPropagator { /// How many public methods mutate this propagator, and so may invalidate a live functional. // cpp/tests/functional_validity.cpp static_asserts that its table has a row for each, so bumping // this when adding a mutator breaks that build until the new method's effect on a functional is - // recorded. The roster: build_graph, propagate, contract_partially, update_initial_operator, - // set_parameter_mapping, update_cutoff, update_cutoff_type, update_basis_change, - // update_lower_atol, update_upper_atol. + // recorded. That table is the roster; keeping the names here too would be a copy nothing enforces. static constexpr size_t num_mutating_methods{10}; auto logical_num_modes() const -> size_t { return logical_num_modes_; } diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index a33a0571..9d790825 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -1082,11 +1082,14 @@ auto MonomialPropagator::expectation_value_and_gradient(const VecD &pa template auto MonomialPropagator::contract_partially(const VecD ¶meters, bool inplace) -> VecD { + // The one site name all three bump paths below report; bump_structure_ stores the pointer, so it has + // to be a literal that outlives every propagator. + static constexpr const char *kInplaceSite = "contract_partially(inplace=true)"; if (partition_group_) { auto merged = concat_partitions_([&](MonomialPropagator &s) { return s.contract_partially(parameters, inplace); }); if (inplace) { - bump_structure_("contract_partially(inplace=true)"); + bump_structure_(kInplaceSite); } return merged; } @@ -1113,7 +1116,7 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo mp_op_.state_coeffs = evolved_state; // The graph lost the layers it folded and the state moved, so every plan built against // either is stale. Out of place nothing changes, so there is no bump on that branch. - bump_structure_("contract_partially(inplace=true)"); + bump_structure_(kInplaceSite); } else { evolved_state = @@ -1129,7 +1132,7 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo const MPGraph sliced = graph_.slice_graph(num_majoranas, true); evolved_op = evolve_operator_with_recompute_(VecD(op), sliced.replay_view(), mapped_params); mp_op_.op_coeffs = evolved_op; - bump_structure_("contract_partially(inplace=true)"); + bump_structure_(kInplaceSite); } else { evolved_op = evolve_operator_with_recompute_(VecD(op), graph_.slice_view(num_majoranas), mapped_params); diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index db2b68b0..64632ccc 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -134,9 +134,10 @@ enum class Outcome : std::uint8_t { struct MutatorRow { std::string_view method; // the public method this row covers void (*apply)(Prop &); - bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted - Outcome exact; // pare_threshold == nullopt, in either picture - Outcome pared; // pare_threshold == kPareThreshold, Heisenberg + bool needs_empty_graph; // build the functional with no graph, so the mutator is accepted + // Paring only changes the verdict where the keep-set came from the operator coefficients, so the + // Schrodinger-pared column is the only one that can differ from the general one. + Outcome outcome; // no pare_threshold in either picture, or pared Heisenberg Outcome pared_schrodinger; // pare_threshold == kPareThreshold, Schrodinger: pares from `op` std::string_view rationale; }; @@ -145,32 +146,28 @@ constexpr std::array kMutatorTable{ MutatorRow{.method = "build_graph", .apply = &mutate_build_graph, .needs_empty_graph = false, - .exact = Outcome::Stale, - .pared = Outcome::Stale, + .outcome = Outcome::Stale, .pared_schrodinger = Outcome::Stale, .rationale = "Appending a layer moves the structure revision, which a pared plan reads as " "readily as an exact one."}, MutatorRow{.method = "propagate", .apply = &mutate_propagate, .needs_empty_graph = true, - .exact = Outcome::Stale, - .pared = Outcome::Stale, + .outcome = Outcome::Stale, .pared_schrodinger = Outcome::Stale, .rationale = "Re-evolves the operator in place. It leaves the layer count at zero, so the " "revision is the only thing that sees it."}, MutatorRow{.method = "contract_partially", .apply = &mutate_contract_partially, .needs_empty_graph = false, - .exact = Outcome::Stale, - .pared = Outcome::Stale, + .outcome = Outcome::Stale, .pared_schrodinger = Outcome::Stale, .rationale = "Consumes the folded layers and rewrites the coefficients. Only inplace=true " "bumps; see contract_partially_out_of_place_keeps_functional_valid."}, MutatorRow{.method = "update_initial_operator", .apply = &mutate_update_initial_operator, .needs_empty_graph = false, - .exact = Outcome::Refreshes, - .pared = Outcome::Refreshes, + .outcome = Outcome::Refreshes, .pared_schrodinger = Outcome::RefusesRefresh, .rationale = "A re-weight moves no structure, so the functional follows the new " "coefficients -- unless its keep-set was thresholded from those very " @@ -178,44 +175,38 @@ constexpr std::array kMutatorTable{ MutatorRow{.method = "set_parameter_mapping", .apply = &mutate_set_parameter_mapping, .needs_empty_graph = false, - .exact = Outcome::Stale, - .pared = Outcome::Stale, + .outcome = Outcome::Stale, .pared_schrodinger = Outcome::Stale, .rationale = "Relabels the layers in place, which changes neither the layer count nor the " "operator -- the revision is the only thing that sees it."}, MutatorRow{.method = "update_cutoff", .apply = &mutate_update_cutoff, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, + .outcome = Outcome::Answers, .pared_schrodinger = Outcome::Answers, .rationale = "Intended: a cutoff gates the next build and changes nothing the plan holds."}, MutatorRow{.method = "update_cutoff_type", .apply = &mutate_update_cutoff_type, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, + .outcome = Outcome::Answers, .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_basis_change", .apply = &mutate_update_basis_change, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, + .outcome = Outcome::Answers, .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_lower_atol", .apply = &mutate_update_lower_atol, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, + .outcome = Outcome::Answers, .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, MutatorRow{.method = "update_upper_atol", .apply = &mutate_update_upper_atol, .needs_empty_graph = false, - .exact = Outcome::Answers, - .pared = Outcome::Answers, + .outcome = Outcome::Answers, .pared_schrodinger = Outcome::Answers, .rationale = "Intended: as update_cutoff."}, }; @@ -250,10 +241,7 @@ auto make_call(Prop &prop, bool gradient, std::optional pare_threshold) } auto expected_outcome(const MutatorRow &row, bool schrodinger, std::optional pare_threshold) -> Outcome { - if (!pare_threshold.has_value()) { - return row.exact; - } - return schrodinger ? row.pared_schrodinger : row.pared; + return pare_threshold.has_value() && schrodinger ? row.pared_schrodinger : row.outcome; } auto run_row(const MutatorRow &row, bool schrodinger, bool gradient, std::optional pare_threshold) -> void { diff --git a/src/monoprop/bindings/binder.h b/src/monoprop/bindings/binder.h index ac9b5c17..1908c15a 100644 --- a/src/monoprop/bindings/binder.h +++ b/src/monoprop/bindings/binder.h @@ -50,6 +50,23 @@ auto cutoff_type_enum_2_str(CutoffType cutoff_type) -> std::string; auto basis_str_2_enum(const std::string &basis) -> Basis; auto basis_enum_2_str(Basis basis) -> std::string; +// One functional class, opaque and non-constructible from Python: the only way to one is the matching +// factory on the propagator, which keep_alive-pins the propagator it borrows from. Both kinds expose the +// same handle surface (see monoprop::detail::FunctionalHandle), so only the call's result differs. +template +auto bind_functional(nb::module_ &mod, const std::string &name, const char *call_doc) -> void { + nb::class_(mod, name.c_str()) + .def("__call__", &Functional::operator(), "parameters"_a, call_doc) + .def_prop_ro( + "num_params", + [](const Functional &f) { return f.num_params(); }, + "Parameter-axis length this functional was built against") + .def_prop_ro( + "follows_weights", + [](const Functional &f) { return f.follows_weights(); }, + "Whether a call after update_initial_operator() answers for the new weights"); +} + template auto bind_monomial_propagator(nb::module_ &mod) -> void { using namespace monoprop; @@ -57,33 +74,13 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { auto name = std::format("MonomialPropagator{:03d}", NumModes); auto cls = nb::class_>(mod, name.c_str()); - // The functional objects. Opaque and non-constructible from Python: the only way to one is the - // matching factory below, which keep_alive-pins the propagator it borrows from. - auto ev_name = std::format("ExpectationValueFunctional{:03d}", NumModes); - nb::class_>(mod, ev_name.c_str()) - .def("__call__", - &ExpectationValueFunctional::operator(), - "parameters"_a, - "Expectation value at the given variational parameters") - .def_prop_ro("num_params", - &ExpectationValueFunctional::num_params, - "Parameter-axis length this functional was built against") - .def_prop_ro("follows_weights", - &ExpectationValueFunctional::follows_weights, - "Whether a call after update_initial_operator() answers for the new weights"); - - auto grad_name = std::format("ExpectationValueAndGradientFunctional{:03d}", NumModes); - nb::class_>(mod, grad_name.c_str()) - .def("__call__", - &ExpectationValueAndGradientFunctional::operator(), - "parameters"_a, - "(expectation value, gradient) at the given variational parameters") - .def_prop_ro("num_params", - &ExpectationValueAndGradientFunctional::num_params, - "Parameter-axis length this functional was built against") - .def_prop_ro("follows_weights", - &ExpectationValueAndGradientFunctional::follows_weights, - "Whether a call after update_initial_operator() answers for the new weights"); + bind_functional>(mod, + std::format("ExpectationValueFunctional{:03d}", NumModes), + "Expectation value at the given variational parameters"); + bind_functional>( + mod, + std::format("ExpectationValueAndGradientFunctional{:03d}", NumModes), + "(expectation value, gradient) at the given variational parameters"); cls.def( "__init__", diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index 0313d8a2..d08f6e7a 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -43,7 +43,7 @@ from .utils import validate_basis_change if TYPE_CHECKING: - from collections.abc import Callable, Sequence + from collections.abc import Sequence from typing import Self from mpi4py import MPI @@ -59,8 +59,8 @@ class _BoundFunctional: """One engine functional, called through the front end's parameter binding. A call has to resolve named or circuit-carried parameters first, so the caller cannot be handed - the engine functional itself. Forwarding everything else by attribute, rather than copying named - properties across, keeps this in step with the bindings on its own. + the engine functional itself. The two engine attributes worth exposing are forwarded by name, so + this class -- not whatever the bindings happen to carry -- is what defines the Python surface. """ def __init__(self, propagator: MonomialPropagator, functional: object) -> None: @@ -70,12 +70,15 @@ def __init__(self, propagator: MonomialPropagator, functional: object) -> None: def __call__(self, parameters: ParameterValues = None) -> float: return self._functional(self._propagator._bind(parameters)) - def __getattr__(self, name: str) -> object: - # Private names are answered by the instance dictionary alone: reaching the engine object for - # one would recurse here through `self._functional` before __init__ has set it. - if name.startswith("_"): - raise AttributeError(name) - return getattr(self._functional, name) + @property + def num_params(self) -> int: + """Parameter-axis length this functional was built against.""" + return self._functional.num_params + + @property + def follows_weights(self) -> bool: + """Whether a call after [update_initial_operator][] answers for the new coefficients.""" + return self._functional.follows_weights class _BoundGradientFunctional(_BoundFunctional): @@ -410,7 +413,7 @@ def gradient( def expectation_value_functional( self, pare_threshold: float | None = None - ) -> Callable[..., float]: + ) -> _BoundFunctional: """Return a reusable callable computing the expectation value from parameters. The callable is built against the graph present now, so a structural change to it -- @@ -445,7 +448,7 @@ def expectation_value_functional( def expectation_value_and_gradient_functional( self, pare_threshold: float | None = None - ) -> Callable[..., tuple]: + ) -> _BoundGradientFunctional: """Return a reusable callable computing (expectation value, gradient). Like [expectation_value_functional][], but one backward pass also yields the gradient. It is @@ -500,7 +503,7 @@ def expval_and_grad( def expval_functional( self, pare_threshold: float | None = None - ) -> Callable[..., float]: + ) -> _BoundFunctional: """Shorthand for [expectation_value_functional][]. See [expectation_value_functional][] for full documentation. @@ -509,7 +512,7 @@ def expval_functional( def expval_and_grad_functional( self, pare_threshold: float | None = None - ) -> Callable[..., tuple]: + ) -> _BoundGradientFunctional: """Shorthand for [expectation_value_and_gradient_functional][]. See [expectation_value_and_gradient_functional][] for full documentation. diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index 9b3dab08..fc59aee2 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -301,7 +301,9 @@ def _mutate_lower_atol(mp): def _mutate_upper_atol(mp): mp.upper_atol = 1e-3 - # (method, mutator, needs_empty_graph, exact, pared, pared_schrodinger, rationale). + # (method, mutator, needs_empty_graph, outcome, pared_schrodinger, rationale). Paring only changes + # the verdict where the keep-set came from the operator coefficients, so the Schrodinger-pared + # column is the only one that can differ from the general one -- as in the C++ table. # "stale" = the call must throw that the propagator moved, "answers" = it must return exactly what # it returned before the mutation, "refreshes" = it must return what a functional built after the # mutation returns, "refuses-refresh" = it must throw that it cannot follow the new weights. @@ -312,7 +314,6 @@ def _mutate_upper_atol(mp): False, "stale", "stale", - "stale", "Appending a layer moves the structure revision, which a pared plan reads as readily as " "an exact one.", ), @@ -322,7 +323,6 @@ def _mutate_upper_atol(mp): True, "stale", "stale", - "stale", "Re-evolves the operator in place. It leaves the layer count at zero, so the revision " "is the only thing that sees it.", ), @@ -332,7 +332,6 @@ def _mutate_upper_atol(mp): False, "stale", "stale", - "stale", "Consumes the folded layers and rewrites the coefficients. Only inplace=True bumps.", ), ( @@ -340,7 +339,6 @@ def _mutate_upper_atol(mp): "_mutate_update_initial_operator", False, "refreshes", - "refreshes", "refuses-refresh", "A re-weight moves no structure, so the functional follows the new coefficients -- " "unless its keep-set was thresholded from those very coefficients, which is " @@ -352,7 +350,6 @@ def _mutate_upper_atol(mp): False, "stale", "stale", - "stale", "Relabels the layers in place, which changes neither the layer count nor the operator -- " "the revision is the only thing that sees it.", ), @@ -362,7 +359,6 @@ def _mutate_upper_atol(mp): False, "answers", "answers", - "answers", "Intended: a cutoff gates the next build and changes nothing the plan holds.", ), ( @@ -371,7 +367,6 @@ def _mutate_upper_atol(mp): False, "answers", "answers", - "answers", "Intended: as cutoff.", ), ( @@ -380,7 +375,6 @@ def _mutate_upper_atol(mp): False, "answers", "answers", - "answers", "Intended: as cutoff.", ), ( @@ -389,7 +383,6 @@ def _mutate_upper_atol(mp): False, "answers", "answers", - "answers", "Intended: as cutoff.", ), ( @@ -398,7 +391,6 @@ def _mutate_upper_atol(mp): False, "answers", "answers", - "answers", "Intended: as cutoff.", ), ) @@ -430,8 +422,7 @@ def test_mutator_effect_on_live_functional( method, mutator, needs_empty_graph, - exact, - pared_outcome, + outcome, pared_schrodinger, rationale, ) = row @@ -447,9 +438,7 @@ def test_mutator_effect_on_live_functional( before = _value(functional(parameters)) getattr(self, mutator)(mp) - expected = exact - if pared: - expected = pared_schrodinger if schrodinger else pared_outcome + expected = pared_schrodinger if pared and schrodinger else outcome context = f"{method}: {rationale}" if expected == "stale": with pytest.raises(RuntimeError, match=r"MP object has been modified"): From adddab152fdad1654ada5fda1c5de22d4727cdbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 12:08:49 +0000 Subject: [PATCH 11/14] chore: reformat --- cpp/monoprop/detail/functional/Control.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index 348c403e..e710c209 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -37,8 +37,8 @@ struct OperatorWeights { // The validity block a propagator shares with every functional plan it makes. // -// A plan borrows from its propagator, so before it reads any of those handles it needs two facts: whether the propagator is still there, and whether the structure the -// snapshot describes is still the propagator's. +// A plan borrows from its propagator, so before it reads any of those handles it needs two facts: whether the +// propagator is still there, and whether the structure the snapshot describes is still the propagator's. // // A copied propagator gets its own block: a copy carries no functionals, so it starts at revision 0. struct FunctionalControl { From db7dfc67f7d1e169646717a0d4e40f67897b1025 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 12:55:59 +0000 Subject: [PATCH 12/14] chore: clean up code comments --- cpp/include/monoprop/Functional.h | 100 ++++++------------ cpp/monoprop/Validation.cpp | 8 +- cpp/monoprop/Validation.h | 28 +++-- cpp/monoprop/detail/functional/Control.h | 41 +++---- .../MonomialPropagator.inl | 51 +++------ src/monoprop/bindings/binder.h | 7 +- 6 files changed, 78 insertions(+), 157 deletions(-) diff --git a/cpp/include/monoprop/Functional.h b/cpp/include/monoprop/Functional.h index dcef6d81..bef5ceca 100644 --- a/cpp/include/monoprop/Functional.h +++ b/cpp/include/monoprop/Functional.h @@ -38,57 +38,43 @@ class MonomialPropagator; namespace detail { -/// One propagator snapshot a functional replays, plus the checks that say the snapshot is still that -/// propagator's own. -/// -/// Immutable once built and held by `shared_ptr`, so one plan backs either functional kind: it -/// holds the snapshot, not the choice of what to compute. Every field is either owned or, where the -/// comment says so, borrowed from the propagator — which is why a functional must not outlive it. +/// Immutable, shared functional replay plan. +/// Borrowed fields require the functional to outlive neither its propagator nor its partitions. template class FunctionalPlan { public: /// A single-partition propagator's snapshot: one replay of its graph against its operator. struct Local { - // The weights this plan was built over, and the fallback when the propagator has published no - // newer set. The same object the control block holds, not a copy of it, so the pointers compare - // equal until a re-weight publishes -- which is how a call sees that it has weights to follow. + // Build-time weights; also used until a re-weight publishes new weights. std::shared_ptr weights; - // Owns its rows and snapshots the term count: the operator's sparse rows grow by push_back as - // terms are appended, so a view would both dangle and outrun the weights' `op`. + // Owned snapshot: operator rows can grow, but `op` cannot. EvalState state; ///< the contraction partner, sparse (Heisenberg) or dense (Schrodinger) VecZ parameter_mapping; ///< optimizer order: which parameter drives graph layer i VecD gen_coeffs; ///< optimizer order, parallel to parameter_mapping - // Always owned, never a view on the propagator's graph_: `cos` holds a raw CosMask pointer per - // layer, and only layers the plan owns are safe from a later append, slice or compaction. + // Owned because `cos` holds raw pointers into graph layers. std::shared_ptr graph; - // The folds keep raw column pointers into the propagator's inverted index, so this plan must not - // outlive the propagator either. + // `cos` borrows columns from the propagator's inverted index. CosCallbacks cos; mpi::Comm comm{}; ///< real MPI across nodes, or the in-process comm across partitions - // The operator-layout backstop. `mp_op` is borrowed and read only after the alive flag says the - // propagator is still there; the other two are what the borrowed inverted index was built over. - // Compared before any use of that index, so a mutation that forgot its revision bump still - // reports staleness rather than folding a rebuilt index through a pointer to the old one. + // Borrowed operator and inverted-index identity check. + // This catches a missing revision bump before the stale index is used. const MPOperator *mp_op{nullptr}; const OperatorIndex *op_store{nullptr}; size_t inverted_index_rows{0}; - // Whether `graph`'s keep-set was thresholded from the operator coefficients, which is Schrodinger - // with a pare threshold and nothing else. Such a plan cannot follow a re-weight: the new - // coefficients select a different keep-set. Heisenberg pares the state, which a re-weight leaves - // alone, so it follows exactly. + // A coefficient-pared Schrodinger graph cannot follow a re-weight. bool pared_from_operator{false}; }; /// A partition facade's snapshot: one child plan per partition, replayed together. struct Fanout { - // Borrowed, like every other field: the group belongs to the facade propagator. + // Borrowed from the facade propagator. partition::PartitionGroup *group{nullptr}; std::vector> partitions; ///< in partition order }; - /// `control` is the propagator's own block; the plan pins the revision it is built at. + /// Pins the propagator control block and its current revision. FunctionalPlan(size_t num_params, std::shared_ptr control, Local local) : num_params_(num_params), control_(std::move(control)), @@ -101,28 +87,22 @@ class FunctionalPlan { expected_revision_(control_->structure_revision.load()), shape_(std::move(fanout)) {} - /// The parameter-axis length the plan was built against; a call must supply exactly this many. + /// Required parameter-axis length. auto num_params() const -> size_t { return num_params_; } - /// Whether a call after a re-weight answers for the new coefficients, rather than throwing. + /// Whether calls may follow re-weighted coefficients. auto follows_weights() const -> bool { if (const auto *fanout = std::get_if(&shape_)) { - // Every child was built in the same picture at the same threshold, so one child answers for - // all of them. Reading an immutable child field needs no fan-out. + // Child plans share picture and threshold. return fanout->partitions.front()->follows_weights(); } return !std::get(shape_).pared_from_operator; } - /// Throw unless `params` fits and the propagator still holds what the plan replays. - // A facade checks its own control block here -- the group it fans out over belongs to the facade -- - // and each child plan then checks its own partition's, on that partition's master thread. Only the - // single-partition shape has an operator to run the layout backstop against. + /// Throw unless `params` and the propagator still match this plan. + // The facade validates its group; child plans validate on their partition masters. auto validate(const VecD ¶ms) const -> void { - // Aliveness is settled here rather than left to validate_functional_state: the layout backstop - // reads the propagator's operator, and every argument is evaluated before the callee runs, so a - // dead propagator has to drop out of the argument list itself. The control block is shared, so it - // stays readable after the propagator is gone -- nothing else the plan holds does. + // Check liveness before reading the borrowed operator for the layout check. const bool alive = control_->propagator_alive.load(); const auto *local = alive ? std::get_if(&shape_) : nullptr; validate_functional_state({.propagator_alive = alive, @@ -133,20 +113,19 @@ class FunctionalPlan { validate_functional_call(params, num_params_); } - /// Replay the snapshot: `fn(request, comm, cos)` locally, or partition 0's answer on a facade. + /// Replay locally, or return partition 0's facade result. template > auto evaluate(Fn &&fn, const VecD ¶ms) const -> R { validate(params); if (const auto *fanout = std::get_if(&shape_)) { - // Each partition allreduces internally, so partition 0 already carries the global answer. - // The fan-out must reach every master: the partitions' collectives are barrier-synced. + // Every partition must join its synchronized collective; partition 0 has the result. return std::move(partition::collect_on_all(*fanout->group, [&](int r) -> R { return fanout->partitions[static_cast(r)]->evaluate(fn, params); })[0]); } const auto &local = std::get(shape_); - // Held for the whole call: `weights` is what keeps the vector `request.op` refers to alive. + // Keeps `request.op` alive for the call. const auto weights = resolve_weights(local); return fn(EvalRequest{.e_core = weights->core_term, .state = local.state, @@ -160,17 +139,10 @@ class FunctionalPlan { } private: - // The weights this call evaluates against: the propagator's live set, which a re-weight replaces - // between two calls. One load, so `op` and `core_term` cannot come from two publications. - // - // A functional is therefore a live view of the weights, not a frozen number: the same parameters give - // the new answer after a re-weight. Everything else a re-weight cannot leave intact -- a store row it - // would have to add, a graph it would have to re-pare -- either throws in - // MPOperator::update_initial_operator or is caught by validate_weight_refresh. + // Load matching `op` and `core_term` from the current weight publication. auto resolve_weights(const Local &local) const -> std::shared_ptr { auto published = control_->weights.load(); - // Null cannot happen -- make_plan_ publishes -- and identity means no re-weight since this plan - // was built. Either way `local.weights` is the current set, so there is nothing to check. + // No publication or no re-weight: build-time weights are current. if (published == nullptr || published == local.weights) { return local.weights; } @@ -180,8 +152,7 @@ class FunctionalPlan { return published; } - // Read straight off the borrowed operator, never through inverted_index(): that accessor rebuilds a - // stale index, which is a write, and a plan must not write to its propagator. + // Do not call inverted_index(): it could rebuild the borrowed index. static auto operator_layout_unchanged(const Local &local) -> bool { return local.mp_op->store.get() == local.op_store && local.mp_op->inverted_index_.has_value() && local.mp_op->inverted_index_->rows() == local.inverted_index_rows; @@ -193,16 +164,14 @@ class FunctionalPlan { std::variant shape_; }; -/// The handle half both functional kinds share: the plan they replay, and the two facts a caller can -/// ask about it without calling it. The kinds differ only in what `operator()` computes. +/// Shared functional handle; derived types differ only in `operator()`. template class FunctionalHandle { public: - /// The parameter-axis length this functional was built against. + /// Required parameter-axis length. auto num_params() const -> size_t { return plan_->num_params(); } - /// True unless a MonomialPropagator::update_initial_operator() makes a call throw: the contract as - /// this object holds it, so a caller need not re-derive it from picture and pare threshold. + /// Whether calls may follow a re-weight. auto follows_weights() const -> bool { return plan_->follows_weights(); } protected: @@ -213,16 +182,10 @@ class FunctionalHandle { } // namespace detail -/// A reusable expectation value over one propagator snapshot: `fn(parameters) -> double`. -/// -/// Built by MonomialPropagator::expectation_value_functional(). It borrows from the propagator that -/// made it (see detail::FunctionalPlan), so it must not outlive it, and a structural change to the -/// propagator makes a call throw rather than answer. +/// Reusable expectation value: `fn(parameters) -> double`. /// -/// A re-weight is not a structural change: the functional follows the propagator's current -/// initial-operator weights, so two calls with the same parameters give two answers across a -/// MonomialPropagator::update_initial_operator(). Build the functional again after the last re-weight -/// to freeze a value. +/// Borrows its propagator and throws after structural mutation. It follows re-weighted initial-operator +/// coefficients unless its graph was coefficient-pared. template class ExpectationValueFunctional : public detail::FunctionalHandle { public: @@ -235,9 +198,8 @@ class ExpectationValueFunctional : public detail::FunctionalHandle { : detail::FunctionalHandle(std::move(plan)) {} }; -/// As ExpectationValueFunctional, plus the gradient from the same backward pass: -/// `fn(parameters) -> (value, gradient)`, the gradient in parameter-axis order. It follows the -/// initial-operator weights on the same terms. +/// Expectation value and gradient from one backward pass. +/// `fn(parameters) -> (value, gradient)`, with the gradient in parameter-axis order. template class ExpectationValueAndGradientFunctional : public detail::FunctionalHandle { public: diff --git a/cpp/monoprop/Validation.cpp b/cpp/monoprop/Validation.cpp index 24f1be5d..aa7cde97 100644 --- a/cpp/monoprop/Validation.cpp +++ b/cpp/monoprop/Validation.cpp @@ -20,21 +20,19 @@ namespace monoprop { -// Every validate_* precondition on a caller-supplied argument reports through this one type. +// Invalid caller input reports through this type. class ValidationError : public std::runtime_error { public: using std::runtime_error::runtime_error; }; -// The propagator was mutated after a functional captured what it replays: a rebuilt graph leaves the -// functional's parameter mapping describing a graph that is gone, a re-weight leaves its snapshotted -// operator coefficients stale. +// A functional no longer matches its propagator. class StaleFunctionalGraph : public std::runtime_error { public: using std::runtime_error::runtime_error; }; -// Declared in Validation.h and used across translation units, so internal linkage does not apply. +// Exported validators below have external linkage. // NOLINTBEGIN(misc-use-internal-linkage) namespace { diff --git a/cpp/monoprop/Validation.h b/cpp/monoprop/Validation.h index 1d9146b0..7e7cf941 100644 --- a/cpp/monoprop/Validation.h +++ b/cpp/monoprop/Validation.h @@ -23,15 +23,11 @@ namespace monoprop { -// Each of these throws when the stated condition does not hold: ValidationError for inconsistent -// arguments, StaleFunctionalGraph when the propagator was mutated after a functional captured the -// graph and operator it replays. Both derive from std::runtime_error, so catching that still catches -// either. +// Validators throw ValidationError for invalid arguments or StaleFunctionalGraph for stale plans. monoprop_EXPORT auto validate_coefficient_lengths(const VecZ ¶meter_mapping, const VecD &gen_coeffs) -> void; -// gate_indices records which ingested gate each monomial came from: one entry per monomial, forming -// contiguous runs from 0. +// gate_indices has one contiguous-run index per ingested monomial. monoprop_EXPORT auto validate_gate_indices(const VecZ &gate_indices, size_t num_monomials) -> void; // params must have max(parameter_mapping)+1 entries. @@ -39,22 +35,22 @@ monoprop_EXPORT auto validate_parameters_length(const VecD ¶ms, const VecZ & monoprop_EXPORT auto validate_functional_call(const VecD ¶meters, size_t expected_num_params) -> void; -/// What a functional must be able to say about its propagator before it reads anything it borrowed. +/// Propagator state required before reading a functional's borrowed data. struct FunctionalState { - bool propagator_alive; ///< false once ~MonomialPropagator has run - size_t current_revision; ///< the propagator's structure revision now - size_t expected_revision; ///< the revision the functional was built at - bool operator_layout_unchanged; ///< the borrowed inverted index still spans the same store and rows - const char *last_structural_change; ///< the method that last bumped the revision, or nullptr + bool propagator_alive; ///< False after destruction begins. + size_t current_revision; ///< Current structure revision. + size_t expected_revision; ///< Build-time structure revision. + bool operator_layout_unchanged; ///< Borrowed index still matches its store and rows. + const char *last_structural_change; ///< Last mutation, or nullptr. }; monoprop_EXPORT auto validate_functional_state(const FunctionalState &state) -> void; -/// The inputs to the check that a functional may follow a newer set of initial-operator weights. +/// Inputs for checking whether a functional may follow new weights. struct WeightRefresh { - size_t weights_revision; ///< the structure revision the newer weights were published at - size_t expected_revision; ///< the revision the functional was built at - bool may_follow_weights; ///< the functional's own follows_weights(), so the rule has one definition + size_t weights_revision; ///< Revision at publication. + size_t expected_revision; ///< Build-time revision. + bool may_follow_weights; ///< Functional weight-following policy. }; monoprop_EXPORT auto validate_weight_refresh(const WeightRefresh &refresh) -> void; diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index e710c209..74b0a9e0 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -22,45 +22,30 @@ namespace monoprop::detail { -// The initial-operator weights an evaluation runs against, published as one immutable set. +// Immutable initial-operator weights, published together on re-weight. // -// A re-weight writes only these two fields: MPOperator::update_initial_operator cannot add a store row, -// and apply_initial_operator_ writes core_term_ beside it, so the store, the inverted index and the graph -// all stay put. That is what lets a functional follow a re-weight instead of going stale. A published set -// is never edited, so one atomic load gives a caller a pair that belong together -- `op` and `core_term` -// read separately could come from two re-weights. +// Re-weighting preserves the store, inverted index and graph. A single atomic load keeps `op` and +// `core_term` from the same publication. struct OperatorWeights { - VecD op; // un-evolved operator coefficients, one per store row - double core_term{0.0}; // the identity term, added to the summed expectation value - size_t structure_revision{0}; // the FunctionalControl revision these weights were published at + VecD op; // One coefficient per store row. + double core_term{0.0}; // Identity contribution to the expectation value. + size_t structure_revision{0}; // Revision at publication. }; -// The validity block a propagator shares with every functional plan it makes. -// -// A plan borrows from its propagator, so before it reads any of those handles it needs two facts: whether the -// propagator is still there, and whether the structure the snapshot describes is still the propagator's. -// -// A copied propagator gets its own block: a copy carries no functionals, so it starts at revision 0. +// Shared validity state for a propagator and its functionals. +// A copy has a fresh block because it has no functionals. struct FunctionalControl { - // Bumped by every change to what a plan replays -- the graph's layers, their parameter labels, or - // the operator's rows. Deliberately NOT bumped by the settings that only gate the next build (the - // atols, the cutoff, the cutoff type, the basis change): none of them touches a plan's snapshot. + // Bumped when a replayed graph layer, parameter label, or operator row changes. std::atomic structure_revision{0}; - // Cleared by ~MonomialPropagator, which runs before its members go away. A plan that outlives its - // propagator must report that instead of reading through handles into freed memory. + // Cleared before propagator members are destroyed. std::atomic propagator_alive{true}; - // The method that last bumped structure_revision, so the error can name it. Always a string - // literal, whose lifetime outlives every propagator. + // String literal naming the last structural change. std::atomic last_structural_change{nullptr}; - // The live initial-operator weights, republished by every re-weight and by the first plan built at a - // given revision. Null until the first plan is built: a propagator with no functionals has nobody to - // publish for, and publishing is a copy of `op`. - // - // Written only by the propagator, on its own thread (a facade publishes through for_each_partition_, - // so each partition publishes on its own pinned master). Read by a call through the plan. + // Current weights. Null until the first functional plan is built. + // The propagator writes; functional calls read. std::atomic> weights{}; }; diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 9d790825..e0a6e3bb 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -110,8 +110,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope } const size_t n_partitions = resolve_partition_count_(partitions, comm); - // The R ranks x S partitions form one flat P = R*S SPMD world, so a mismatch across ranks would - // deadlock at the first hybrid collective. + // All ranks need the same partition count for hybrid collectives. if (comm.kind == mpi::Comm::Kind::Mpi && mpi::size(comm) > 1 && mpi::allreduce_sum(n_partitions, comm) != n_partitions * static_cast(mpi::size(comm))) { throw PartitionCountMismatch( @@ -164,15 +163,15 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope auto op = schrodinger_ ? generate_paired_op(sc / 2 + sc % 2, logical_num_modes_) : local_heisenberg_terms; const size_t expected_local_terms = std::max(1, op.size() / std::max(1, num_ranks)); - // Must run before the store: packed_inline_width_() derives the packed-row width from cutoff_fn_. + // packed_inline_width_() depends on cutoff_fn_. regenerate_cutoff_fn_(); mp_op_.store = std::make_unique>(packed_inline_width_()); mp_op_.store->reserve(expected_local_terms); - // Store replaced: drop the stale lazy inverted index so it rebuilds against the new store. + // Rebuild the lazy index for the new store. mp_op_.inverted_index_.reset(); size_t i = 0; - // The initial monomials are distinct, so emplace (insert-if-absent) is an assigning insert here. + // Initial monomials are distinct, so emplace assigns here. for (size_t r = 0; r < op.size(); ++r) { const auto &mono = materialize_row(op, r); if (my_rank == find_rank(mono, num_ranks)) { @@ -189,9 +188,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope template MonomialPropagator::~MonomialPropagator() { - // Before any member goes away, so a functional that outlives this propagator reports the destruction - // instead of reading through the handles it borrowed. On a facade this also runs before - // partition_group_ is destroyed, so a fanned-out plan sees the facade dead first. + // Mark borrowed state unavailable before members are destroyed. functional_control_->propagator_alive.store(false); } @@ -392,9 +389,7 @@ auto MonomialPropagator::apply_initial_operator_(const OperatorDict &o return applied; } catch (...) { - // A part-applied re-weight is not one a functional may follow: the loop above can throw with - // core_term_ already written, and a facade can have applied some partitions. Nothing was published, - // so the bump is what stops a functional answering from weights the propagator disagrees with. + // A failed partial re-weight cannot be followed. bump_structure_("update_initial_operator()"); throw; } @@ -402,8 +397,7 @@ auto MonomialPropagator::apply_initial_operator_(const OperatorDict &o template auto MonomialPropagator::publish_weights_() -> std::shared_ptr { - // get_operator() merges the pending init_op_map terms and so is a write: legal here, on the - // propagator's own thread, and never from a plan. + // get_operator() merges pending terms and must run on the propagator thread. auto weights = std::make_shared( detail::OperatorWeights{.op = mp_op_.get_operator(), .core_term = core_term_, @@ -414,8 +408,7 @@ auto MonomialPropagator::publish_weights_() -> std::shared_ptr auto MonomialPropagator::weights_for_plan_() -> std::shared_ptr { - // A publication stamped with the current revision is still current: every mutation that can move the - // coefficients bumps, and a re-weight republishes. + // Weights from the current revision are still current. if (auto published = functional_control_->weights.load(); published != nullptr && published->structure_revision == functional_control_->structure_revision.load()) { return published; @@ -974,13 +967,11 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho using Plan = detail::FunctionalPlan; if (partition_group_) { - // One fan-out for both functional kinds: the plan holds the snapshot, not the choice of what to - // compute. The children are built on the partitions' own masters, where their state lives. + // Both functional types share this fan-out plan. typename Plan::Fanout fanout; fanout.group = partition_group_.get(); fanout.partitions = map_partitions_([&](MonomialPropagator &s) { return s.make_plan_(pare_threshold); }); - // The children hold the same graph structure and gate info, so one of them already carries the - // parameter-axis length. A facade validates nothing else itself (see FunctionalPlan::validate). + // Child plans share graph structure and parameter-axis length. const auto num_params = fanout.partitions.front()->num_params(); return std::make_shared(num_params, functional_control_, std::move(fanout)); } @@ -992,9 +983,7 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.gen_coeffs = std::move(gate_arrays.second); const auto num_params = expected_num_params(local.parameter_mapping); - // Nothing here needs a dense state: energy only dots it against the evolved operator, and the gradient - // scatters it into its own thread-local scratch before back-evolving. So Heisenberg hands over just the - // sparse scores; Schrödinger's state is the live evolved vector, snapshotted whole. + // Heisenberg snapshots sparse scores; Schrodinger snapshots the dense state. const auto num_terms = mp_op_.size(); local.state = [&] { if (schrodinger_) { @@ -1011,15 +1000,10 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho local.op_store = mp_op_.store.get(); local.inverted_index_rows = inverted_index.rows(); - // Only this combination pares against the coefficients a re-weight replaces, so only it must refuse to - // follow one. + // Only coefficient-pared Schrodinger plans reject re-weighting. local.pared_from_operator = schrodinger_ && pare_threshold.has_value(); - // The plan always owns its layers, so `cos` -- which holds a raw CosMask pointer per layer -- points - // into layers no later append_layer, slice_graph or maybe_compact_layers can move or free. 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. Only a - // pared graph carries masks, and pare_graph builds those into an owned graph anyway. + // Own graph layers because `cos` stores raw pointers into them. if (pare_threshold.has_value()) { auto full_cos_of_layer = [this, &inverted_index](size_t i) -> CosMask { const auto layer = graph_.get_layer_traversal(i); @@ -1027,7 +1011,7 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho const auto combined = detail::make_fold_cache(inverted_index, gen, layer.scaled_count(), basis_); return detail::fold_to_cos_mask(combined); }; - // Threshold the picture's driving vector: the Hamiltonian in Schrödinger, the state otherwise. + // Threshold the Hamiltonian in Schrodinger, otherwise the state. const auto keep = schrodinger_ ? indices_above(local.weights->op, *pare_threshold) : local.state.indices_above(*pare_threshold); const auto count = schrodinger_ ? local.weights->op.size() : local.state.length(); @@ -1035,8 +1019,7 @@ auto MonomialPropagator::make_plan_(std::optional pare_thresho std::make_shared(pare_graph(graph_, keep, count, schrodinger_, comm_, full_cos_of_layer)); } else { - // Rebuilt from the active layers rather than copied off graph_: MPGraph keeps what a slice - // retired in front of front_offset_, and a plan has no use for those layers. + // Snapshot active layers only; graph_ retains layers retired by slicing. std::vector owned; owned.reserve(graph_.layers()); for (size_t i = 0; i < graph_.layers(); ++i) { @@ -1065,7 +1048,7 @@ auto MonomialPropagator::expectation_value_and_gradient_functional(std template auto MonomialPropagator::expectation_value(const VecD ¶meters) -> double { if (partition_group_) { - // Each partition allreduces internally, so every partition returns the global value; take partition 0. + // Each partition returns the same allreduced value. return map_partitions_([&](MonomialPropagator &s) { return s.expectation_value(parameters); })[0]; } return expectation_value_functional(std::nullopt)(parameters); @@ -1074,7 +1057,7 @@ auto MonomialPropagator::expectation_value(const VecD ¶meters) -> template auto MonomialPropagator::expectation_value_and_gradient(const VecD ¶meters) -> std::pair { if (partition_group_) { - // As in expectation_value(): the gradient is allreduced inside each partition. + // Each partition returns the same allreduced gradient. return map_partitions_([&](MonomialPropagator &s) { return s.expectation_value_and_gradient(parameters); })[0]; } return expectation_value_and_gradient_functional(std::nullopt)(parameters); diff --git a/src/monoprop/bindings/binder.h b/src/monoprop/bindings/binder.h index 1908c15a..f6e26f0b 100644 --- a/src/monoprop/bindings/binder.h +++ b/src/monoprop/bindings/binder.h @@ -50,9 +50,7 @@ auto cutoff_type_enum_2_str(CutoffType cutoff_type) -> std::string; auto basis_str_2_enum(const std::string &basis) -> Basis; auto basis_enum_2_str(Basis basis) -> std::string; -// One functional class, opaque and non-constructible from Python: the only way to one is the matching -// factory on the propagator, which keep_alive-pins the propagator it borrows from. Both kinds expose the -// same handle surface (see monoprop::detail::FunctionalHandle), so only the call's result differs. +// Opaque Python functional types, created only by their propagator factories. template auto bind_functional(nb::module_ &mod, const std::string &name, const char *call_doc) -> void { nb::class_(mod, name.c_str()) @@ -159,8 +157,7 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { "parameters"_a, "Expectation value and its gradient at the given variational parameters"); - // keep_alive<0, 1>: the functional borrows this propagator's inverted index and, without a pare - // threshold, its graph, so the propagator must outlive it. Python has no other way to know. + // Functionals borrow this propagator's index and graph. cls.def("expectation_value_functional", &MonomialPropagator::expectation_value_functional, "pare_threshold"_a = std::nullopt, From c474dd7cba93d1745508ceb0cf2087df3fa5bbba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 17:25:35 +0000 Subject: [PATCH 13/14] fix: issues raised in review --- AGENTS.md | 22 +- cpp/include/monoprop/MonomialPropagator.h | 12 +- cpp/monoprop/detail/functional/Control.h | 58 ++++- .../MonomialPropagator.inl | 137 +++++++----- cpp/tests/functional_validity.cpp | 154 ++++++++++++- docs/content/docs/features/evaluation.mdx | 4 +- src/monoprop/bindings/binder.h | 3 +- src/monoprop/monomial_propagator.py | 49 ++-- tests/test_parameter_validation.py | 209 ++++++++++++++++-- 9 files changed, 531 insertions(+), 117 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e6e90677..7495aa82 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,11 +114,15 @@ Key files: the propagator still there, and does it still hold what I replay?" without dereferencing it. **Every new mutating method must call `bump_structure_("its_name()")`** once the mutation has committed (a rejected mutation must not bump); the settings that only gate the next build — the atols, the cutoff, - the cutoff type, the basis change — deliberately do not. A plan additionally re-derives the operator's - store pointer and inverted-index row count as a backstop, so a missing bump reports staleness instead - of folding a rebuilt index. The block also carries the published `OperatorWeights`: a re-weight - publishes a new set rather than bumping, which is what lets a live functional follow it, and bumps only - if it fails part-way. Publishing runs on the propagator's own thread (a facade publishes through + the cutoff type, the basis change — deliberately do not. Two halves go with that success-path bump: + guard the mutation itself with `bump_structure_on_unwind_` (`detail::BumpOnUnwind`), constructed after + the last rejection check and before the first write, so a mutator that throws part-way still + invalidates; and do not bump a fan-out whose children all no-op — decide with the facade-transparent + readers *before* fanning out, or `monoprop_PARTITIONS=auto` invalidates where `off` does not. A plan + additionally re-derives the operator's store pointer and inverted-index row count as a backstop, so a + missing bump reports staleness instead of folding a rebuilt index. The block also carries the + published `OperatorWeights`: a re-weight publishes a new set rather than bumping, which is what lets a + live functional follow it, and bumps only if it fails part-way. Publishing runs on the propagator's own thread (a facade publishes through `for_each_partition_`), and a plan reads the set once per call so `op` and `core_term` cannot come from two publications. - **The partition facade**: `partitions > 1` makes a `MonomialPropagator` a facade over S single-partition @@ -195,10 +199,10 @@ mp = MajoranaPropagator(operator, initial_state, cutoff=4) 7. Add Python bindings in `src/monoprop/bindings/binder.h` 8. Regenerate bindings with `tools/generate-binders.py` 9. Test with both C++ and Python tests -10. If the new method mutates a `MonomialPropagator`: call `bump_structure_` from it (see - `detail::FunctionalControl`), bump `MonomialPropagator::num_mutating_methods`, and add its row to - the mutation table (see "Testing Structure"). A mutator with no row leaves its effect on a live - functional unrecorded. +10. If the new method mutates a `MonomialPropagator`: call `bump_structure_` from it and guard it with + `bump_structure_on_unwind_` (see `detail::FunctionalControl`), bump + `MonomialPropagator::num_mutating_methods`, and add its row to the mutation table (see "Testing + Structure"). A mutator with no row leaves its effect on a live functional unrecorded. ## Documentation Maintenance Policy diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index 4bdc61d7..7adf5fe0 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -282,6 +282,8 @@ class MonomialPropagator { auto evolved_operator_terms(const VecD ¶meters, double atol) -> std::vector>>; + /// Re-weight the initial operator in place; every existing term op_dict omits is zeroed, the + /// identity term included. virtual auto update_initial_operator(const OperatorDict &op_dict) -> void { apply_initial_operator_(op_dict); } protected: @@ -330,9 +332,13 @@ class MonomialPropagator { // so it must not invalidate anything. A mutation that can fail part-way bumps on the failure path // instead: invalidating a functional that did not need it costs a rebuild, answering from a snapshot // of a half-written operator costs a wrong number. - auto bump_structure_(const char *site) -> void { - functional_control_->last_structural_change.store(site); - functional_control_->structure_revision.fetch_add(1); + auto bump_structure_(const char *site) const -> void { functional_control_->bump(site); } + + // The failure-path half of the rule above: guard the mutation itself, so a mutator that throws + // after committing part of its work still invalidates. `armed` is false where the guarded call + // is a known no-op. See detail::BumpOnUnwind. + auto bump_structure_on_unwind_(const char *site, bool armed = true) const -> detail::BumpOnUnwind { + return detail::BumpOnUnwind(*functional_control_, site, armed); } // Hand the current initial-operator weights to every functional over this propagator, as one diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index 74b0a9e0..1cde70a0 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -16,37 +16,73 @@ #include #include +#include #include #include "monoprop/TypeAliases.h" namespace monoprop::detail { -// Immutable initial-operator weights, published together on re-weight. -// -// Re-weighting preserves the store, inverted index and graph. A single atomic load keeps `op` and -// `core_term` from the same publication. +// Immutable initial-operator weights, atomically published on re-weight. +// A single load keeps `op` and `core_term` from one publication. struct OperatorWeights { VecD op; // One coefficient per store row. double core_term{0.0}; // Identity contribution to the expectation value. size_t structure_revision{0}; // Revision at publication. }; -// Shared validity state for a propagator and its functionals. -// A copy has a fresh block because it has no functionals. +// Validity state shared by a propagator and its functionals. +// Copies start with a fresh block. struct FunctionalControl { - // Bumped when a replayed graph layer, parameter label, or operator row changes. + // Bumped when replayed structure changes. std::atomic structure_revision{0}; - // Cleared before propagator members are destroyed. + // Cleared before propagator destruction. std::atomic propagator_alive{true}; - // String literal naming the last structural change. + // Last structural change, as a string literal. std::atomic last_structural_change{nullptr}; - // Current weights. Null until the first functional plan is built. - // The propagator writes; functional calls read. + // Current weights, null until the first functional plan. Written by the propagator and read by + // functional calls. std::atomic> weights{}; + + // Record a replayed-structure change. `site` must outlive the propagator. + auto bump(const char *site) -> void { + last_structural_change.store(site); + structure_revision.fetch_add(1); + } +}; + +// Bumps the control block when a mutation throws after it may have changed replayed state. +// Construct after validation and before the first write. Set `armed` to false for known no-ops; +// the caller records successful changes. +class [[nodiscard]] BumpOnUnwind { +public: + // `site` must outlive the propagator. + BumpOnUnwind(FunctionalControl &control, const char *site, bool armed = true) + : control_(control), + site_(site), + armed_(armed), + uncaught_(std::uncaught_exceptions()) {} + + BumpOnUnwind(const BumpOnUnwind &) = delete; + BumpOnUnwind(BumpOnUnwind &&) = delete; + auto operator=(const BumpOnUnwind &) -> BumpOnUnwind & = delete; + auto operator=(BumpOnUnwind &&) -> BumpOnUnwind & = delete; + + ~BumpOnUnwind() { + // This also works when called during another unwind. + if (armed_ && std::uncaught_exceptions() > uncaught_) { + control_.bump(site_); + } + } + +private: + FunctionalControl &control_; + const char *site_; + bool armed_; + int uncaught_; }; } // namespace monoprop::detail diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index e0a6e3bb..aaf10779 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -352,47 +352,49 @@ auto MonomialPropagator::packed_inline_width_() const -> size_t { template auto MonomialPropagator::apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD> { - try { - if (partition_group_) { - // The facade holds no local terms of its own, so the return is empty. Each partition publishes - // its own weights on its own master, which is what its children's functionals read. - for_each_partition_([&](MonomialPropagator &s) { s.update_initial_operator(op_dict); }); - return {}; - } - const size_t num_ranks = static_cast(mpi::size(comm_)); - const size_t my_rank = static_cast(mpi::rank(comm_)); - - OperatorDict new_op; - for (const auto &[ind, coeff] : op_dict) { - const auto mono = indices_to_bitset_checked(ind, 2 * logical_num_modes_); - if (ind.empty()) { // Core term, store in all - core_term_ = algebra_encode_coeff(basis_, coeff, mono); - continue; - } - if (my_rank == find_rank(mono, num_ranks)) { - const auto mono_indices = bitset_to_indices(mono); - new_op[mono_indices] = coeff; - } - } + // A failed re-weight cannot be followed: the facade fan-out can commit some partitions before + // another rejects a term only it holds, and a functional cannot tell that apart from a dict + // rejected before anything committed. Guarding the whole body keeps today's verdict for both. + auto guard = bump_structure_on_unwind_("update_initial_operator()"); + if (partition_group_) { + // The facade holds no local terms of its own, so the return is empty. Each partition publishes + // its own weights on its own master, which is what its children's functionals read. + for_each_partition_([&](MonomialPropagator &s) { s.update_initial_operator(op_dict); }); + return {}; + } + const size_t num_ranks = static_cast(mpi::size(comm_)); + const size_t my_rank = static_cast(mpi::rank(comm_)); - // No revision bump: a re-weight leaves the store, the inverted index and the graph where they are, - // so a live functional follows the new coefficients instead of going stale. Publication comes after - // the commit, so a functional never sees half of one. - auto applied = mp_op_.update_initial_operator(new_op, schrodinger_); - // Only refresh a set somebody has already been given: a null one means no plan was ever built, - // and weights_for_plan_() publishes on demand, so there is nothing left stale by skipping this. - // Publishing copies the whole coefficient vector, which a re-weight loop would otherwise pay per - // call on a propagator with no functionals. - if (functional_control_->weights.load() != nullptr) { - publish_weights_(); + OperatorDict new_op; + // A dict that omits the identity term means zero, which is what MPOperator::update_initial_operator + // does with every row this dict leaves out; accumulated locally so a rejected dict leaves the + // propagator's own expectation value where it was. + double core_term = 0.0; + for (const auto &[ind, coeff] : op_dict) { + const auto mono = indices_to_bitset_checked(ind, 2 * logical_num_modes_); + if (ind.empty()) { // Core term, store in all + core_term = algebra_encode_coeff(basis_, coeff, mono); + continue; + } + if (my_rank == find_rank(mono, num_ranks)) { + const auto mono_indices = bitset_to_indices(mono); + new_op[mono_indices] = coeff; } - return applied; } - catch (...) { - // A failed partial re-weight cannot be followed. - bump_structure_("update_initial_operator()"); - throw; + + // No revision bump: a re-weight leaves the store, the inverted index and the graph where they are, + // so a live functional follows the new coefficients instead of going stale. Publication comes after + // the commit, so a functional never sees half of one. + auto applied = mp_op_.update_initial_operator(new_op, schrodinger_); + core_term_ = core_term; + // Only refresh a set somebody has already been given: a null one means no plan was ever built, + // and weights_for_plan_() publishes on demand, so there is nothing left stale by skipping this. + // Publishing copies the whole coefficient vector, which a re-weight loop would otherwise pay per + // call on a propagator with no functionals. + if (functional_control_->weights.load() != nullptr) { + publish_weights_(); } + return applied; } template @@ -665,17 +667,21 @@ auto MonomialPropagator::build_graph(const std::vector &majorana std::optional parameters, std::optional only_rotate_len_k) -> void { validate_only_rotate_len_k_(only_rotate_len_k, 2 * logical_num_modes_); + // Hoisted above the fan-out so a call that appends nothing bumps on neither shape: the children + // re-run both identically, so deciding here moves nothing observable. + if (majoranas.empty()) { + return; + } + validate_coefficient_lengths(parameter_mapping, gen_coeffs); if (partition_group_) { + // A fan-out that throws part-way has already mutated the partitions it reached. + auto guard = bump_structure_on_unwind_("build_graph()"); for_each_partition_([&](MonomialPropagator &s) { s.build_graph(majoranas, parameter_mapping, gen_coeffs, gate_indices, parameters, only_rotate_len_k); }); bump_structure_("build_graph()"); return; } - if (majoranas.empty()) { - return; - } - validate_coefficient_lengths(parameter_mapping, gen_coeffs); VecZ local_gates; if (gate_indices.has_value()) { @@ -691,15 +697,14 @@ auto MonomialPropagator::build_graph(const std::vector &majorana g += gate_offset; } - if (!parameters.has_value()) { - evolve_mode_build_graph_(majoranas, parameter_mapping, gen_coeffs, local_gates, only_rotate_len_k); - } - else { + // The seed is computed before the guard below: it validates and contracts out of place, so a + // rejection here has changed nothing yet. + VecD seed; + if (parameters.has_value()) { // map_params() indexes `parameters` by parameter_mapping, so a too-short vector reads out of bounds. validate_parameters_length(*parameters, parameter_mapping); // Coefficient-informed build: seed by contracting the existing graph so atol truncation sees // realistic coefficients. That graph covers the parameter prefix [0, m). - VecD seed; if (graph_layers() > 0) { const auto existing = graph_gate_arrays_(); const size_t m = expected_num_params(existing.first); @@ -720,6 +725,15 @@ auto MonomialPropagator::build_graph(const std::vector &majorana else { seed = current_picture_coeffs_(); } + } + + // The gate loop bounds-checks each generator as it reaches it, so a bad generator in a multi-gate + // call throws with the earlier layers already appended. + auto guard = bump_structure_on_unwind_("build_graph()"); + if (!parameters.has_value()) { + evolve_mode_build_graph_(majoranas, parameter_mapping, gen_coeffs, local_gates, only_rotate_len_k); + } + else { evolve_mode_graph_with_coeffs_(majoranas, parameter_mapping, gen_coeffs, @@ -738,13 +752,8 @@ auto MonomialPropagator::propagate(const std::vector &majoranas, const VecD ¶meters, std::optional only_rotate_len_k) -> void { validate_only_rotate_len_k_(only_rotate_len_k, 2 * logical_num_modes_); - if (partition_group_) { - for_each_partition_([&](MonomialPropagator &s) { - s.propagate(majoranas, parameter_mapping, gen_coeffs, parameters, only_rotate_len_k); - }); - bump_structure_("propagate()"); - return; - } + // Hoisted above the fan-out so a call that evolves nothing bumps on neither shape: the children + // re-run all three identically (graph_layers() is facade-transparent), so nothing observable moves. if (majoranas.empty()) { return; } @@ -757,6 +766,18 @@ auto MonomialPropagator::propagate(const std::vector &majoranas, "build_graph() to extend it.", graph_layers())); } + if (partition_group_) { + // A fan-out that throws part-way has already mutated the partitions it reached. + auto guard = bump_structure_on_unwind_("propagate()"); + for_each_partition_([&](MonomialPropagator &s) { + s.propagate(majoranas, parameter_mapping, gen_coeffs, parameters, only_rotate_len_k); + }); + bump_structure_("propagate()"); + return; + } + // The gate loop bounds-checks each generator as it reaches it, so a bad generator in a multi-gate + // call throws with the earlier gates already folded into the operator. + auto guard = bump_structure_on_unwind_("propagate()"); evolve_mode_contract_immediately_(majoranas, parameter_mapping, gen_coeffs, parameters, only_rotate_len_k); bump_structure_("propagate()"); } @@ -1069,9 +1090,14 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo // to be a literal that outlives every propagator. static constexpr const char *kInplaceSite = "contract_partially(inplace=true)"; if (partition_group_) { + // Decided before the fan-out, so an empty parameter vector -- which every child early-returns on + // without folding anything -- leaves a functional valid here exactly as it does on one partition. + validate_parameters_length(parameters, parameter_mapping()); + const bool folds = !parameters.empty(); + auto guard = bump_structure_on_unwind_(kInplaceSite, inplace && folds); auto merged = concat_partitions_([&](MonomialPropagator &s) { return s.contract_partially(parameters, inplace); }); - if (inplace) { + if (inplace && folds) { bump_structure_(kInplaceSite); } return merged; @@ -1087,6 +1113,9 @@ auto MonomialPropagator::contract_partially(const VecD ¶meters, bo } const size_t num_majoranas = parameter_mapping.size(); + // Retiring the folded layers commits before the evolve that can throw, so the failure path has to + // bump as well; out of place nothing is written, so the guard stays disarmed there. + auto guard = bump_structure_on_unwind_(kInplaceSite, inplace); // Inplace slicing produces an owned MPGraph that must be bound to a named local before viewing // (never view a temporary); slice_view() views this graph's still-live layers directly. if (schrodinger_) { diff --git a/cpp/tests/functional_validity.cpp b/cpp/tests/functional_validity.cpp index 64632ccc..69766744 100644 --- a/cpp/tests/functional_validity.cpp +++ b/cpp/tests/functional_validity.cpp @@ -30,6 +30,7 @@ #include #include "monoprop/MonomialPropagator.h" +#include "monoprop/detail/functional/Control.h" #include "monoprop/detail/mpi/MPICompat.h" using namespace monoprop; @@ -51,11 +52,22 @@ const std::vector kBaseGates{VecZ{0}, VecZ{2}}; // answer exactly like one built with it from the start, so both sides read it from here. constexpr double kReweightedFirstWeight = 2.75; +// The identity weight the core-term cases start from; every other case leaves the row out entirely, +// which is what makes a re-weight that also leaves it out a no-op there. +constexpr double kCoreTerm = 0.25; + // `partitions` is passed explicitly, so it wins over the suite-wide monoprop_PARTITIONS=off. -auto make_propagator(bool schrodinger, size_t partitions = 1, double first_weight = 1.0) -> Prop { +// `core_term` adds the identity row, which only the core-term cases below need. +auto make_propagator(bool schrodinger, + size_t partitions = 1, + double first_weight = 1.0, + std::optional core_term = std::nullopt) -> Prop { OperatorDict initial_ham; initial_ham[VecZ{0, 1}] = std::complex{0.0, first_weight}; initial_ham[VecZ{2, 3}] = std::complex{0.0, 0.5}; + if (core_term.has_value()) { + initial_ham[VecZ{}] = std::complex{*core_term, 0.0}; + } const auto cutoff = static_cast(2 * kNumModes); return Prop(initial_ham, cutoff, @@ -551,3 +563,143 @@ BOOST_AUTO_TEST_CASE(building_another_functional_is_not_a_reweight) { BOOST_TEST(call(kBaseParams) == before); } + +// The scope guard behind the failure-path rule, over a bare control block: a scope that returns +// normally leaves the revision alone, and one that throws bumps it once, recording its site. +BOOST_AUTO_TEST_CASE(bump_on_unwind_bumps_only_when_the_scope_throws) { + detail::FunctionalControl control; + + { + auto guard = detail::BumpOnUnwind(control, "site()"); + } + BOOST_TEST(control.structure_revision.load() == 0U); + BOOST_TEST((control.last_structural_change.load() == nullptr)); + + BOOST_CHECK_THROW( + [&] { + auto guard = detail::BumpOnUnwind(control, "site()"); + throw std::runtime_error("part-way"); + }(), + std::runtime_error); + BOOST_TEST(control.structure_revision.load() == 1U); + BOOST_TEST(std::string_view(control.last_structural_change.load()) == std::string_view("site()")); + + // Disarmed is how a known no-op opts out, so it must stay silent on both paths. + BOOST_CHECK_THROW( + [&] { + auto guard = detail::BumpOnUnwind(control, "other()", /*armed=*/false); + throw std::runtime_error("part-way"); + }(), + std::runtime_error); + BOOST_TEST(control.structure_revision.load() == 1U); +} + +// A gate generator is bounds-checked only as the gate loop reaches it, so a bad one in a multi-gate +// call throws with the earlier gates already committed: valid, out of range, valid appends before it +// throws in both pictures (Heisenberg walks the gates in reverse). +namespace { + +const std::vector kPartWayGates{VecZ{0}, VecZ{2 * kNumModes + 1}, VecZ{2}}; +const VecZ kPartWayMapping{0, 1, 2}; +const VecD kPartWayCoeffs{1.0, 1.0, 1.0}; + +auto reports_site(std::string_view site) { + return [site](const std::runtime_error &e) { + const std::string_view what(e.what()); + BOOST_TEST_INFO("message: " << what); + return what.find(site) != std::string_view::npos; + }; +} + +} // namespace + +BOOST_AUTO_TEST_CASE(a_part_way_build_graph_failure_invalidates_the_functional) { + for (const bool schrodinger : {false, true}) { + BOOST_TEST_CONTEXT("schrodinger=" << schrodinger) { + auto prop = make_propagator(schrodinger); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(kBaseParams)); + const size_t layers_before = prop.graph_layers(); + + BOOST_CHECK_THROW(prop.build_graph(kPartWayGates, kPartWayMapping, kPartWayCoeffs), std::runtime_error); + + // The graph grew, so the call did mutate on its way to the throw. + BOOST_TEST(prop.graph_layers() > layers_before); + BOOST_CHECK_EXCEPTION(call(kBaseParams), std::runtime_error, reports_site("build_graph()")); + } + } +} + +// propagate() folds into the operator instead of appending, so nothing counts the mutation: without +// the failure-path bump the functional would keep answering for coefficients that had moved. +BOOST_AUTO_TEST_CASE(a_part_way_propagate_failure_invalidates_the_functional) { + for (const bool schrodinger : {false, true}) { + BOOST_TEST_CONTEXT("schrodinger=" << schrodinger) { + auto prop = make_propagator(schrodinger); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + BOOST_CHECK_NO_THROW(call(VecD{})); + + BOOST_CHECK_THROW(prop.propagate(kPartWayGates, kPartWayMapping, kPartWayCoeffs, VecD{0.3, 0.7, 0.5}), + std::runtime_error); + + BOOST_CHECK_EXCEPTION(call(VecD{}), std::runtime_error, reports_site("propagate()")); + } + } +} + +// A call that appends or folds nothing is not a mutation. The facade decides that before it fans out, +// so both partition counts must agree -- this is the off/auto divergence the table exists to rule out. +BOOST_AUTO_TEST_CASE(no_op_mutators_keep_a_functional_valid) { + for (const size_t partitions : {1U, 2U}) { + BOOST_TEST_CONTEXT("partitions=" << partitions) { + auto prop = make_propagator(/*schrodinger=*/false, partitions); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + const double before = call(VecD{}); + + // No graph, so there is nothing to fold and no layer to retire; the return is the current + // coefficients either way, which is what it was before the call. + const VecD folded = prop.contract_partially(VecD{}, /*inplace=*/true); + BOOST_TEST(folded == prop.contract_partially(VecD{}, /*inplace=*/false), tt::per_element()); + prop.build_graph({}, VecZ{}, VecD{}); + prop.propagate({}, VecZ{}, VecD{}, VecD{}); + BOOST_TEST(prop.graph_layers() == 0U); + + BOOST_TEST(call(VecD{}) == before, tt::tolerance(1e-12)); + } + } +} + +// The core term describes the dict that committed, like every other row: a re-weight that leaves the +// identity out zeroes it, which is what MPOperator::update_initial_operator does with the rows the dict +// omits. The live functional and the propagator's own answer must both see that. +BOOST_AUTO_TEST_CASE(a_reweight_that_drops_the_core_term_zeroes_it) { + auto prop = make_propagator(/*schrodinger=*/false, /*partitions=*/1, /*first_weight=*/1.0, kCoreTerm); + build_base_graph(prop); + auto call = make_call(prop, /*gradient=*/false, std::nullopt); + BOOST_TEST(prop.core_term() == kCoreTerm); + + mutate_update_initial_operator(prop); // carries no identity row + + BOOST_TEST(prop.core_term() == 0.0); + auto fresh = make_propagator(/*schrodinger=*/false, /*partitions=*/1, kReweightedFirstWeight); + build_base_graph(fresh); + BOOST_TEST(call(kBaseParams) == make_call(fresh, /*gradient=*/false, std::nullopt)(kBaseParams)); + BOOST_TEST(call(kBaseParams) == prop.expectation_value(kBaseParams)); +} + +// The other half of the same rule: a dict the store rejects commits nothing, so the core term stays +// where it was and the propagator's own expectation value is untouched. +BOOST_AUTO_TEST_CASE(a_rejected_reweight_leaves_the_core_term_alone) { + auto prop = make_propagator(/*schrodinger=*/false, /*partitions=*/1, /*first_weight=*/1.0, kCoreTerm); + build_base_graph(prop); + const double before = prop.expectation_value(kBaseParams); + + OperatorDict rejected; + rejected[VecZ{}] = std::complex{2.0, 0.0}; + rejected[VecZ{0, 2}] = std::complex{0.0, 1.0}; // a term the operator does not hold + BOOST_CHECK_THROW(prop.update_initial_operator(rejected), std::runtime_error); + + BOOST_TEST(prop.core_term() == kCoreTerm); + BOOST_TEST(prop.expectation_value(kBaseParams) == before, tt::tolerance(1e-12)); +} diff --git a/docs/content/docs/features/evaluation.mdx b/docs/content/docs/features/evaluation.mdx index 957c93a5..3e24cb68 100644 --- a/docs/content/docs/features/evaluation.mdx +++ b/docs/content/docs/features/evaluation.mdx @@ -72,9 +72,11 @@ This is what every public mutating method does to a functional built before it r | [contract_partially][monoprop.monomial_propagator.MonomialPropagator.contract_partially] with `inplace=True` | raises | | [parameter_mapping][monoprop.monomial_propagator.MonomialPropagator.parameter_mapping] | raises | | [update_initial_operator][monoprop.monomial_propagator.MonomialPropagator.update_initial_operator] | answers for the new coefficients — except in the Schrödinger picture with a `pare_threshold`, where it raises, because the pared graph was selected from the coefficients the re-weight replaced | -| a rejected `update_initial_operator` | raises: the call can fail with the core term already written | +| a rejected `update_initial_operator` | raises: a partitioned propagator applies the re-weight one partition at a time, so a term only one of them holds is rejected with its siblings already re-weighted | +| a `build_graph` or `propagate` that fails part-way | raises: a gate generator is bounds-checked only as the gate loop reaches it, so the gates before it are already committed | | `cutoff`, `cutoff_type`, `basis_change`, `lower_atol`, `upper_atol` | answers, unchanged: these gate the next build and touch nothing a functional replays | | `contract_partially` with `inplace=False` | answers, unchanged: it mutates nothing | +| a call that folds or appends nothing — `contract_partially([], inplace=True)`, `build_graph` or `propagate` with no gates | answers, unchanged: nothing moved, on any partition count | The table is executable — see [the mutation table](/testing#the-mutation-table). diff --git a/src/monoprop/bindings/binder.h b/src/monoprop/bindings/binder.h index f6e26f0b..d3ff8e23 100644 --- a/src/monoprop/bindings/binder.h +++ b/src/monoprop/bindings/binder.h @@ -179,7 +179,8 @@ auto bind_monomial_propagator(nb::module_ &mod) -> void { cls.def("update_initial_operator", &MonomialPropagator::update_initial_operator, "op_dict"_a, - "Rewrite the initial operator from an {indices: coefficient} dict"); + "Rewrite the initial operator from an {indices: coefficient} dict; every existing term " + "the dict omits is zeroed, the identity term included"); cls.def_prop_rw("lower_atol", &MonomialPropagator::lower_atol, diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index d08f6e7a..73bb151f 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -26,7 +26,7 @@ import logging from abc import ABC, abstractmethod -from typing import TYPE_CHECKING, Generic, TypeVar +from typing import TYPE_CHECKING, Generic, Protocol, TypeVar import numpy as np @@ -55,21 +55,30 @@ T_op = TypeVar("T_op", MajoranaOperator, PauliOperator) -class _BoundFunctional: - """One engine functional, called through the front end's parameter binding. +T_ret_co = TypeVar("T_ret_co", covariant=True) - A call has to resolve named or circuit-carried parameters first, so the caller cannot be handed - the engine functional itself. The two engine attributes worth exposing are forwarded by name, so - this class -- not whatever the bindings happen to carry -- is what defines the Python surface. - """ - def __init__(self, propagator: MonomialPropagator, functional: object) -> None: +class _EngineFunctional(Protocol[T_ret_co]): + """Interface exposed by engine functionals.""" + + def __call__(self, parameters: list[float], /) -> T_ret_co: ... + + @property + def num_params(self) -> int: ... + + @property + def follows_weights(self) -> bool: ... + + +class _BoundFunctionalBase(Generic[T_ret_co]): + """Shared parameter binding and forwarded functional attributes.""" + + def __init__( + self, propagator: MonomialPropagator, functional: _EngineFunctional[T_ret_co] + ) -> None: self._propagator = propagator self._functional = functional - def __call__(self, parameters: ParameterValues = None) -> float: - return self._functional(self._propagator._bind(parameters)) - @property def num_params(self) -> int: """Parameter-axis length this functional was built against.""" @@ -81,7 +90,14 @@ def follows_weights(self) -> bool: return self._functional.follows_weights -class _BoundGradientFunctional(_BoundFunctional): +class _BoundFunctional(_BoundFunctionalBase[float]): + """One engine functional returning the expectation value.""" + + def __call__(self, parameters: ParameterValues = None) -> float: + return self._functional(self._propagator._bind(parameters)) + + +class _BoundGradientFunctional(_BoundFunctionalBase[tuple[float, np.ndarray]]): """As the value functional, but returning ``(value, gradient)`` with the gradient as ``float64``.""" def __call__(self, parameters: ParameterValues = None) -> tuple[float, np.ndarray]: @@ -592,13 +608,16 @@ def update_initial_operator(self, new_operator: T_op) -> None: pared graph was selected from the coefficients this call replaces, so it raises instead of following them. - A rejected re-weight invalidates them, since this call can fail with the core term already - written: they raise rather than answer for weights the propagator disagrees with. + A rejected re-weight invalidates them: a partitioned propagator applies the new weights one + partition at a time, so a term only one of them holds is rejected with its siblings already + re-weighted, and a functional cannot tell that apart from a rejection that committed nothing. + They raise rather than answer for weights the propagator disagrees with. Args: new_operator: A [MajoranaOperator][monoprop.majorana.MajoranaOperator] or [PauliOperator][monoprop.pauli.PauliOperator], per the front-end, whose terms replace - the matching initial-operator. + the matching initial-operator. Every existing term it leaves out is zeroed, the + identity term included. Raises: RuntimeError: In the Heisenberg picture, if a term is absent from the current operator. diff --git a/tests/test_parameter_validation.py b/tests/test_parameter_validation.py index fc59aee2..423cc1d8 100644 --- a/tests/test_parameter_validation.py +++ b/tests/test_parameter_validation.py @@ -16,6 +16,7 @@ from __future__ import annotations +import numpy as np import pytest from monoprop import ( @@ -29,9 +30,46 @@ from monoprop.pauli import PauliOperator -def _value(result): - """The value component of a functional's answer, for either functional kind.""" - return result[0] if isinstance(result, tuple) else result +def _components(result): + """A functional's answer as ``(value, gradient)``; ``None`` gradient for the value-only kind.""" + return result if isinstance(result, tuple) else (result, None) + + +def _assert_answers_match(actual, expected, *, exact, context=""): + """Assert both components of two answers agree, bit-exactly or to ``pytest.approx``. + + The two functional kinds return different shapes, so a test parametrized over both has to + compare whatever the kind under test returned -- a gradient dropped here is a gradient nobody + checks. + """ + value, gradient = _components(actual) + expected_value, expected_gradient = _components(expected) + assert (gradient is None) == (expected_gradient is None), context + if exact: + assert value == expected_value, context + if gradient is not None: + assert np.array_equal(gradient, expected_gradient), context + else: + assert value == pytest.approx(expected_value), context + if gradient is not None: + assert gradient == pytest.approx(expected_gradient), context + + +def _assert_answers_differ(actual, other, context=""): + """Assert both components of two answers moved (a gradient counts as moved if any entry did).""" + value, gradient = _components(actual) + other_value, other_gradient = _components(other) + assert value != pytest.approx(other_value), context + if gradient is not None: + assert gradient != pytest.approx(other_gradient), context + + +# The propagator method each functional factory is the reusable form of, so a test can compare a +# functional's answer against the direct call of the same shape. +_DIRECT_CALL = { + "expectation_value_functional": "expectation_value", + "expectation_value_and_gradient_functional": "expectation_value_and_gradient", +} def _two_gate_graph(serial_comm): @@ -197,16 +235,19 @@ def test_functional_follows_initial_operator_update( ) functional = getattr(mp, functional_name)() parameters = [0.3, 0.7] - before = _value(functional(parameters)) + before = functional(parameters) mp.update_initial_operator(updated_operator) # A re-weight moves no structure, so the functional built before it follows the new # coefficients instead of refusing the call: it now answers what the propagator answers. - after = _value(functional(parameters)) - assert after != pytest.approx(before) - assert after == pytest.approx(mp.expval(parameters)) - assert after == _value(getattr(mp, functional_name)()(parameters)) + after = functional(parameters) + _assert_answers_differ(after, before) + direct = getattr(mp, _DIRECT_CALL[functional_name])(parameters) + _assert_answers_match(after, direct, exact=False) + _assert_answers_match( + after, getattr(mp, functional_name)()(parameters), exact=True + ) class TestFunctionalValidityTable: @@ -240,11 +281,16 @@ def _base_circuit(cls): ) @classmethod - def _propagator(cls, comm, *, schrodinger, with_graph, first_weight=1.0): + def _propagator( + cls, comm, *, schrodinger, with_graph, first_weight=1.0, core_term=None + ): + # The identity row is only carried by the core-term case; every other case leaves it out, + # which is what makes a re-weight that also leaves it out a no-op there. + terms = {(0, 1): first_weight * 1j, (2, 3): 0.5j} + if core_term is not None: + terms[()] = core_term mp = MajoranaPropagator( - MajoranaOperator( - {(0, 1): first_weight * 1j, (2, 3): 0.5j}, num_modes=cls._MODES - ), + MajoranaOperator(terms, num_modes=cls._MODES), [0, 1], cutoff=cls._CUTOFF, schrodinger_cutoff=cls._CUTOFF if schrodinger else None, @@ -435,7 +481,7 @@ def test_mutator_effect_on_live_functional( threshold = self._PARE_THRESHOLD if pared else None functional = getattr(mp, functional_name)(threshold) - before = _value(functional(parameters)) + before = functional(parameters) getattr(self, mutator)(mp) expected = pared_schrodinger if pared and schrodinger else outcome @@ -447,12 +493,14 @@ def test_mutator_effect_on_live_functional( with pytest.raises(RuntimeError, match=r"cannot follow the new weights"): functional(parameters) elif expected == "refreshes": - after = _value(functional(parameters)) + after = functional(parameters) fresh = getattr(mp, functional_name)(threshold) - assert after == _value(fresh(parameters)), context - assert after != pytest.approx(before), context + _assert_answers_match(after, fresh(parameters), exact=True, context=context) + _assert_answers_differ(after, before, context=context) else: - assert _value(functional(parameters)) == pytest.approx(before), context + _assert_answers_match( + functional(parameters), before, exact=False, context=context + ) @pytest.mark.parametrize("partitions", ["off", "auto"]) @pytest.mark.parametrize( @@ -537,7 +585,7 @@ def test_reweighted_functional_matches_a_fresh_propagator( mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) functional = getattr(mp, functional_name)(threshold) - before = _value(functional(parameters)) + before = functional(parameters) self._mutate_update_initial_operator(mp) fresh = self._propagator( @@ -546,11 +594,11 @@ def test_reweighted_functional_matches_a_fresh_propagator( with_graph=True, first_weight=self._REWEIGHTED_FIRST_WEIGHT, ) - expected = _value(getattr(fresh, functional_name)(threshold)(parameters)) + expected = getattr(fresh, functional_name)(threshold)(parameters) - after = _value(functional(parameters)) - assert after == expected - assert after != pytest.approx(before) + after = functional(parameters) + _assert_answers_match(after, expected, exact=True) + _assert_answers_differ(after, before) @pytest.mark.parametrize("partitions", ["off", "auto"]) def test_pared_schrodinger_functional_refuses_to_follow_a_reweight( @@ -574,6 +622,123 @@ def test_pared_schrodinger_functional_refuses_to_follow_a_reweight( pared(parameters) assert exact(parameters) == pytest.approx(mp.expval(parameters)) + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_no_op_mutators_keep_a_functional_valid( + self, monkeypatch, serial_comm, partitions + ): + """A call that appends or folds nothing is not a mutation, on either partition setting. + + The facade decides that before it fans out, so ``off`` and ``auto`` must agree -- an + unconditional bump on the fan-out is exactly the divergence this table exists to rule out. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=False) + functional = mp.expectation_value_functional() + before = functional([]) + + # No graph, so there is nothing to fold and no layer to retire; the return is the current + # coefficients either way. + folded = mp.contract_partially([], inplace=True) + assert folded == pytest.approx(mp.contract_partially([], inplace=False)) + empty = Circuit((), self._MODES, ()) + mp.build_graph(empty) + mp.propagate(empty) + assert mp.n_parameters == 0 + + assert functional([]) == pytest.approx(before) + + # A gate generator is bounds-checked only as the gate loop reaches it, so a bad one in a + # multi-gate call throws with the earlier gates already committed. The front end validates + # generators against num_modes, so the engine is the only way to express one -- as in + # _mutate_basis_change. + _PART_WAY_GATES = ((0,), (2 * _MODES + 1,), (2,)) + _PART_WAY_MAPPING = (0, 1, 2) + _PART_WAY_COEFFS = (1.0, 1.0, 1.0) + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_part_way_build_graph_failure_invalidates_the_functional( + self, monkeypatch, serial_comm, partitions + ): + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=True) + functional = mp.expectation_value_functional() + parameters = list(self._PARAMS) + functional(parameters) + layers_before = mp.graph_layers + + with pytest.raises(RuntimeError): + mp._simulator.build_graph( + self._PART_WAY_GATES, self._PART_WAY_MAPPING, self._PART_WAY_COEFFS + ) + + # The graph grew, so the call did mutate on its way to the throw. + assert mp.graph_layers > layers_before + with pytest.raises(RuntimeError, match=r"build_graph"): + functional(parameters) + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + def test_part_way_propagate_failure_invalidates_the_functional( + self, monkeypatch, serial_comm, partitions + ): + """propagate() folds into the operator instead of appending, so nothing counts the + mutation: without the failure-path bump the functional would keep answering for + coefficients that had moved. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + mp = self._propagator(serial_comm, schrodinger=False, with_graph=False) + functional = mp.expectation_value_functional() + functional([]) + + with pytest.raises(RuntimeError): + mp._simulator.propagate( + self._PART_WAY_GATES, + self._PART_WAY_MAPPING, + self._PART_WAY_COEFFS, + [0.3, 0.7, 0.5], + ) + + with pytest.raises(RuntimeError, match=r"propagate"): + functional([]) + + @pytest.mark.parametrize("partitions", ["off", "auto"]) + @pytest.mark.parametrize( + "functional_name", + [ + "expectation_value_functional", + "expectation_value_and_gradient_functional", + ], + ) + def test_reweight_that_drops_the_core_term_zeroes_it( + self, monkeypatch, serial_comm, functional_name, partitions + ): + """The identity row describes the dict that committed, like every other row: a re-weight + that leaves it out zeroes it, which is what the store does with every row a dict omits. + """ + monkeypatch.setenv("monoprop_PARTITIONS", partitions) + parameters = list(self._PARAMS) + mp = self._propagator( + serial_comm, schrodinger=False, with_graph=True, core_term=0.25 + ) + functional = getattr(mp, functional_name)(None) + before = functional(parameters) + + self._mutate_update_initial_operator(mp) # carries no identity row + + fresh = self._propagator( + serial_comm, + schrodinger=False, + with_graph=True, + first_weight=self._REWEIGHTED_FIRST_WEIGHT, + ) + after = functional(parameters) + _assert_answers_match( + after, getattr(fresh, functional_name)(None)(parameters), exact=True + ) + _assert_answers_match( + after, getattr(mp, _DIRECT_CALL[functional_name])(parameters), exact=False + ) + _assert_answers_differ(after, before) + @pytest.mark.parametrize("partitions", ["off", "auto"]) def test_pared_functional_is_invalidated_by_build_graph( self, monkeypatch, serial_comm, partitions From 1735b4211913f8c7d4307c1c2ebb1b8a9a14ff50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Thu, 20 Aug 2026 17:37:06 +0000 Subject: [PATCH 14/14] fix: use mutex instead of atomic> the latter is not implemented in LLVM libc++ --- cpp/monoprop/detail/functional/Control.h | 48 +++++++++++++++--------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/cpp/monoprop/detail/functional/Control.h b/cpp/monoprop/detail/functional/Control.h index 1cde70a0..5c288a2c 100644 --- a/cpp/monoprop/detail/functional/Control.h +++ b/cpp/monoprop/detail/functional/Control.h @@ -18,48 +18,61 @@ #include #include #include +#include #include "monoprop/TypeAliases.h" namespace monoprop::detail { -// Immutable initial-operator weights, atomically published on re-weight. -// A single load keeps `op` and `core_term` from one publication. +// Initial-operator weights published together. struct OperatorWeights { VecD op; // One coefficient per store row. - double core_term{0.0}; // Identity contribution to the expectation value. - size_t structure_revision{0}; // Revision at publication. + double core_term{0.0}; // Identity contribution. + size_t structure_revision{0}; // Publication revision. }; -// Validity state shared by a propagator and its functionals. -// Copies start with a fresh block. +// Thread-safe slot for the current weights. +class WeightsSlot { +public: + auto load() const -> std::shared_ptr { + const std::lock_guard lock(mutex_); + return weights_; + } + + auto store(std::shared_ptr weights) -> void { + const std::lock_guard lock(mutex_); + weights_ = std::move(weights); + } + +private: + mutable std::mutex mutex_; // Allows reads through const FunctionalControl. + std::shared_ptr weights_; +}; + +// State shared by a propagator and its functionals. struct FunctionalControl { - // Bumped when replayed structure changes. + // Changes when replayed structure changes. std::atomic structure_revision{0}; - // Cleared before propagator destruction. + // Cleared before destruction. std::atomic propagator_alive{true}; - // Last structural change, as a string literal. + // Last structural change. std::atomic last_structural_change{nullptr}; - // Current weights, null until the first functional plan. Written by the propagator and read by - // functional calls. - std::atomic> weights{}; + // Current weights, null until the first plan. + WeightsSlot weights; - // Record a replayed-structure change. `site` must outlive the propagator. + // Record a structure change. auto bump(const char *site) -> void { last_structural_change.store(site); structure_revision.fetch_add(1); } }; -// Bumps the control block when a mutation throws after it may have changed replayed state. -// Construct after validation and before the first write. Set `armed` to false for known no-ops; -// the caller records successful changes. +// Bumps the control block if a mutation throws after a possible state change. class [[nodiscard]] BumpOnUnwind { public: - // `site` must outlive the propagator. BumpOnUnwind(FunctionalControl &control, const char *site, bool armed = true) : control_(control), site_(site), @@ -72,7 +85,6 @@ class [[nodiscard]] BumpOnUnwind { auto operator=(BumpOnUnwind &&) -> BumpOnUnwind & = delete; ~BumpOnUnwind() { - // This also works when called during another unwind. if (armed_ && std::uncaught_exceptions() > uncaught_) { control_.bump(site_); }