expectation_value() rebuilds the entire evaluation functional on every call

[robertodr]

🤖 AI text below 🤖

What

expectation_value and expectation_value_and_gradient construct a fresh evaluation functional on
every single call.

cpp/monoprop/detail/monomial_propagator/MonomialPropagatorImpl.h:989-1004

auto MonomialPropagator<NumModes>::expectation_value(const VecD &parameters) -> double {
    ...
    return expectation_value_functional(std::nullopt)(parameters);
}

auto MonomialPropagator<NumModes>::expectation_value_and_gradient(const VecD &parameters)
    -> std::pair<double, VecD> {
    ...
    return expectation_value_and_gradient_functional(std::nullopt)(parameters);
}

Why this is a problem

make_functional_ (same file, ~lines 885-954) is not cheap. Per call it:

• copies the full coefficient vector — VecD op = mp_op_.get_operator();, one double per operator
  term;
• snapshots the evaluation state (EvalState::sparse copies rows and values; EvalState::dense
  copies the whole vector);
• calls build_cos_callbacks, which allocates a LayerCos per graph layer and constructs a
  LazyFold in each (make_lazy_fold → make_fold_mask + build_even_parity_generator_columns +
  a heap std::vector<size_t> columns).

That is O(terms + layers) of allocation before any evaluation work happens. An optimizer looping on
expectation_value pays it on every iteration — and on a partitioned propagator, once per partition,
since the facade fans out to s.expectation_value(parameters).

The documented workaround is to use expectation_value_functional and hold the callable, but the
convenience methods are the obvious entry point and are what the shorthand expval / expval_and_grad
wrap.

Suggested fix

Memoize the pare-free functional behind a revision counter, invalidated by every mutator. The mutation
surface is small and already funnelled:

• build_graph, propagate
• contract_partially(..., inplace=true)
• apply_initial_operator_ / update_initial_operator
• set_parameter_mapping
• update_setting_ — which already covers the lower_atol, upper_atol, cutoff, cutoff_type and
  basis_change setters

Also bump it from the non-const mp_op() and indexing() accessors. Both hand out mutable
references to the operator store, so a caller can mutate behind the cache's back; a conservative
invalidation on any non-const access is the cheap way to stay correct.

Worth noting what the existing safety net does and does not catch: the functional already calls
validate_expected_graph_layers(graph->layers(), expected_layers) on entry, which detects layer-count
drift from build_graph/contract_partially — but not an operator re-weight. So
update_initial_operator is the case the revision counter must not miss.

Only the pare_threshold == std::nullopt path should be cached; a pared functional is explicitly
per-threshold.

Verification

• just bench serial before/after, comparing benches/results/REPORT.md.
• Add a regression test that mutates the propagator between two expectation_value calls —
  update_initial_operator, an atol setter, set_parameter_mapping — and asserts the second value
  reflects the mutation. That is the bug the cache could introduce, so it should be written before the
  cache.
• just test, just test-mpi "1;2;4".

---

Found by a code-reading review of the repository at 29a8050. No build tree was available, so the
analysis is from source inspection and should be confirmed by measurement.