refactor!: :recycle: drop non-type template parameter for number of modes by robertodr · Pull Request #226 · Algorithmiq/monoprop

robertodr · 2026-08-13T12:28:33Z

🤖 AI text below 🤖

Summary

Drops the NumModes non-type template parameter from the C++ engine. A MonomialPropagator used to be a template instantiated per compile-time mode ceiling, with a generated dispatch table and binder header picking the right instantiation for a given system size. Now there is one compiled MonomialPropagator: the logical width is a runtime constructor argument, and detail::storage_modes_for() sizes monomial storage from it (rounded up to a whole 32-mode block). Bitset becomes a runtime-width value type — the first 8 words inline, wider ones spilling to the heap — in place of a Bitset<NumBits> template.

Removing the compile-time width also removes the per-term speed that if constexpr gave for free, so this PR carries the machinery that earns it back at runtime: a bound-once-per-layer storage word count (with_kernel_width) that turns the per-term word loops back into a compile-time trip count, a per-gate kernel seam (TermProductsFor) between dense and sparse row representations, and a new sparse ("support-form") row store chosen per propagator from the storage width, alongside the existing dense OperatorIndex backend behind the shared RowAccess.h accessor seam.

Changes

Core engine

MonomialPropagator takes one runtime num_modes (no compile-time ceiling, no per-width instantiation); logical_num_modes_/logical_num_modes() renamed to num_modes_/num_modes().
Bitset has runtime-width: inline storage for the first 8 words, heap-spilled beyond that (>250 modes); per-word loops dispatch through detail::with_nwords instead of if constexpr arms.
New sparse row store (detail::SparseRowStore, fixed-width mode lanes + a 2-bit codes word per row) as a third row-accessor backend beside std::vector<Bitset> and OperatorIndex, chosen per propagator by SparseRowStore::preferred_for_modes() from the target ISA (monoprop_SPARSE_ROW_MIN_MODES) and forceable process-wide with monoprop_ROW_STORE=dense|sparse. Backend is bound once per layer via with_store, never per term.
algebra/CodesAlgebra.h: the structural algebra over a sparse row's codes word, one function per dense counterpart, plus sparse_toggle for the per-term product; falls back to the dense product on scratch-row overflow rather than truncating.
layer_build/TermProduct.h: the per-term kernel seam (TermProductsFor<Store, A, W>), SparseTermProducts/DenseTermProducts/DenseTermProductsW<A, W>, and with_kernel_width, which binds the storage word count as a compile-time template parameter once per layer (kNarrowKernelWords caps which widths get an instantiation) and throws KernelWidthMismatch on a width mismatch instead of compiling it away.
New query-record wire format (QueryKeysFor, query_payload_words_for) with a record-count header and a dense escape tail for fully-paired products that overflow a fixed-stride sparse record.
Row store resizing in place (OperatorIndex::resized() / SparseRowStore::resized()) so a cutoff-widening setting change migrates existing rows instead of silently overflowing them.
Generated dispatch table and binder header removed (tools/generate-binders.py, tools/generate-dispatch.py, src/monoprop/bindings/bindings.cpp.in); bindings are now a single, non-generated src/monoprop/bindings/bindings.cpp.

Testing

Wide-system test regime: ModeEmbedding/WIDE_EMBEDDING (Python) and test_utils::embed_case (C++) relabel a narrow fixture's modes into a 260-logical/288-storage-mode system, so the fixture's exact energy/gradient still applies without checking in a wider fixture blob.
New C++ suites: sparse_row_store_tests.cpp, sparse_index_tests.cpp, sparse_wire_tests.cpp, codes_algebra_tests.cpp, codes_product_tests.cpp, term_product_tests.cpp, word_kernel_tests.cpp, row_store_selection_tests.cpp, store_interchange_tests.cpp, wide_system_tests.cpp — the whole suite now also runs under monoprop_ROW_STORE=sparse (the sparse-rows CTest label), plus dedicated MPI CTest variants for the sparse wire format (monoprop_MPI_SPARSE_ROWS_TEST_PROCS).
New Python tests: tests/test_mode_width.py, tests/test_wide_system.py, tests/test_deep_circuit_gradient.py; tests/test_binding_layout.py and tools/_binding_layout.py removed along with the generated binder they checked.
just diff-baseline-sparse added for comparing the two row-store backends (tolerance-based, since they agree on term sets/values but not term order), distinct from the byte-wise just diff-baseline.

Simplification passes (no behavior change; verified byte-identical capture-baseline and tolerant diff-baseline-sparse agreement)

Collapsed Bitset's inline/spilled word-op arms into a shared with_words_/apply_words_ dispatcher; shared row-materialization and reinsertion helpers across the two row-store backends via RowHashTable and detail::geometric_row_capacity; hoisted several loop-invariant computations out of per-term/per-layer hot paths.

Docs

AGENTS.md rewritten to document the new runtime-width architecture (mode width, row-store seam, per-term kernel seam, query record, partition facade); docs/content/docs/building.mdx, testing.mdx, benchmarks.mdx updated to match.

Checklist

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

AI/LLM disclosure

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

Important

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

Warning

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

github-actions · 2026-08-13T13:21:42Z

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

- CMakeLists.txt had two `if(monoprop_ENABLE_CXX_UNIT_TESTS) enable_testing() / include(CTest) endif()` blocks, each justified by a different comment. Keep one, right before add_subdirectory(cpp) where it must run, with both justifications merged into a single comment. - PartitionGroup.h's include-ordering comment still named MonomialPropagator.inl, which this PR renamed to detail/monomial_propagator/MonomialPropagator.cpp. Addresses PR #226 review comments from robertodr. Assisted-by: ClaudeCode:claude-sonnet-5

- SparseQueryKeys::configure() cleared retained_ and retained_lanes_ on a re-extent but not the parallel retained_bases_/retained_escapes_ arrays, desynchronizing retain()'s handles from their backing storage the next time the batch was reused at a new extent. - The sparse row store was sized straight from cutoff_ via slots_for_bound(), bypassing packed_inline_width_()'s Schrödinger special case; whenever ceil(schrodinger_cutoff/2) > cutoff_, every initial Schrödinger row spilled to the overflow map. Factored the shared row-width bound (with the Schrödinger case) into row_width_bound_(), used by both backends. - SparseRowStore::for_each_index's comment claimed "index order"; it walks the hash table's slot array (table/probe order), as the class comment above it already says and as MPOperator::for_each_term's own comment states for the same traversal. - The DenseQueryKeys class comment and its echo in Resolve.h claimed the batch is "grow-only and never cleared between layers" for both of its use sites. That's true only of the thread_local batch in Resolve.h; Engine.h's keys_ is a plain member of a LayerBuildEngine built fresh per build_layer call, so it rebuilds every layer by necessity (retain()'s handles must not survive past their layer). Addresses PR #226 review comments from robertodr. Assisted-by: ClaudeCode:claude-sonnet-5

…ng change update_cutoff()/update_cutoff_type()/update_basis_change() rebuild cutoff_fn_ but never touched the row store, which is sized once at construction from the cutoff-derived bound. Widening the cutoff afterwards silently pushed every future row past the old bound into the overflow map for the rest of the propagator's life, with no diagnostic. - OperatorIndex::resized() / SparseRowStore::resized() rebuild a store at a new row width, migrating every existing row rather than dropping them: each row is re-flowed through set() (which re-decides inline-vs-overflow per row), while the hash index is copied as-is, since the hash depends only on the monomial, never on the row width. Row index i is preserved for every row -- load-bearing, since op_coeffs, state_rows_/state_vals_ and the evolution graph all key off it. - MPOperator::resize_store() dispatches to whichever backend is live; MPOperator::row_width() reads its current width the same way. - MonomialPropagator::target_row_width_() is the one place that picks the ideal width for the live backend, shared by the constructor's initial store setup and by the new resize_row_store_if_needed_(), which the three setters call after regenerate_cutoff_fn_() and which only migrates when the bound actually moved. New tests cover the migration at all three layers (OperatorIndex, SparseRowStore, MPOperator) including rows crossing the overflow boundary in both directions, plus an end-to-end MonomialPropagator check that a propagator built narrow then widened via update_cutoff() matches one built directly at the wide cutoff. Full serial suite (630 cases, dense and sparse-forced) passes. Addresses PR #226 review comment from robertodr. Assisted-by: ClaudeCode:claude-sonnet-5

The sparse backend has its own wire format -- query_payload_words_for's per-backend stride, the escape tail, kOverflowLane, append_escape_tail have no dense counterpart -- and is the backend wide (MPI-scale) systems resolve to, but it previously only ran serially (monoprop_ROW_STORE=sparse forced on the single-rank *_sparse_rows variants). Register an MPI counterpart per rank count so the sparse wire format is actually exercised across ranks. The sparse MPI ranks come from a new, separate cache var, monoprop_MPI_SPARSE_ROWS_TEST_PROCS (default "2"), rather than reusing monoprop_MPI_TEST_PROCS: growing dense-backend rank coverage must not silently multiply how many sparse-row mpiexec launches CI pays for. Addresses PR #226 review comment from Panadestein. Assisted-by: ClaudeCode:claude-sonnet-5

DenseTermProductsW<A, W>'s constructor pairs a generator's own width against W, which with_kernel_width chose separately from the store's row word count -- the one non-hot-loop, once-per-gate place two independently-sourced widths get associated. It only asserted the match, so in Release (NDEBUG) a mismatch would silently read past W words of gen/mono/new_mono instead of failing. Unlike the six per-term word ops in Bitset.h, which stay assert-only on purpose because Release must keep their loops bare, this binding runs once per gate: a real branch here costs nothing next to the per-term work it guards, so it now throws KernelWidthMismatch (std::runtime_error) instead of compiling away. The per-term ops in Bitset.h are unchanged -- addresses only the construction-boundary half of PR #226 review comment from Panadestein. Assisted-by: ClaudeCode:claude-sonnet-5

…fficiency) - Bitset.h: collapse the inline/spilled regime split, hand-copied into eleven word ops with the loop body written once per arm, into one private with_words_ dispatcher (compile-time W inline, runtime count spilled) plus apply_words_ for &=/|=/^=. fused_xor_words and and_fold_words now take the count as a deduced parameter, so WordKernel<W> and Bitset's own arms keep sharing one definition. Add Bitset::words_for so the (num_bits + 63) / 64 in MPOperator and layer_build/Common.h is the constructor's own arithmetic. - RowHashTable: host insert_distinct_range, fold() and spilled_rows_bytes, which OperatorIndex and SparseRowStore spelled identically -- including the unexplained 24-byte map-node estimate that skews operator_memory_breakdown() if corrected in one only. - Give both row stores a common row_width(), so MPOperator forwards it like every other accessor instead of detecting the backend with a requires-expression at the one place a third backend plugs in. Route the scan's five direct store.popcount/for_each_position calls through the row_popcount/for_each_row_position seam row_accessor_tests.cpp gates. - MonomialPropagator: read the backend off the installed store rather than re-deriving it from the environment; move the dense row-width clamp into OperatorIndex::inline_width_for_bound, mirroring slots_for_bound (the missing lower clamp left a bound of 0 never converging, re-migrating every row on each settings change); derive storage_num_modes() from the operator; drop packed_inline_width_ and the nullary target_row_width_. - Delete MajoranaAlgebra/PauliAlgebra's encode_coeff/decode_coeff/ state_phase, unreachable since the point-dispatch ternaries took over, so the basis mapping has one home. Collapse Settings::row_store and row_store_unrecognized into the optional parse_row_store already returns. Make FusedScanResult's escape buffers scan locals, and share the vector<bool> staging of map_partitions_indexed_/collect_on_all via detail::staged_collect. - Efficiency: cutoff_sums masks the inactive window instead of copying the whole monomial per term via `mono >> active_bit_offset` (non-zero offset on both shipping models, and an allocation past kInlineWords); OperatorIndex::resized copies position slots instead of materializing and re-walking a Bitset per row; initial_state_mask drops its intermediate index vector; cutoff_function_basis_change takes the basis by value and moves it; the self-resolve key batch is sized in resolve_range_ rather than once per gate. - Tests: hoist five byte-identical random-monomial generators into test_utils::random_monomial and the two copies of the rows-are-also-findable incantation into test_utils::indexed_operator; codes_algebra_tests uses the store's own sparse_row_to_bitset. No behavior change: 632 CTest cases and the Python suite pass on both backends, and a fresh capture-baseline is byte-identical to HEAD's (diff-baseline-sparse agrees to 1e-10). Skipped, and why, in the PR thread: owner() calling find_rank (adds its n_ranks == 0 branch to the per-term multi-rank path), merging the three test OwnedRow types (three genuinely different shapes), caching ContractSink's state mask (an aggregate with no ctor, so the fix would be a stale-prone width cache), thread-local generator columns (2-4 size_t, two callers can overlap) and exposing SparseRowStore::kMinModes through the bindings (a public API addition). Assisted-by: ClaudeCode:claude-opus-5

Common.h names detail::OperatorIndex unqualified (query_payload_words_for, QueryKeysFor<OperatorIndex>) and gets it transitively through TypeAliases.h, which pulls it in for its own reasons. Include it directly instead of relying on that -- IWYU, and the same fix landed independently upstream once already after TypeAliases.h briefly stopped providing it during a rebase. Assisted-by: ClaudeCode:claude-sonnet-5

… main rebase Two gaps main's own history exposed on this rebase, neither ever touched by a commit already in this branch (so no earlier conflict caught them): - #291 (main) split the row-store accessors out of TypeAliases.h into a new detail/operator/RowAccess.h, still written against the pre-refactor Monomial<NumModes>/OperatorIndex<NumModes> API. This branch never had that file, so its own drop-NumModes commit never converted it. Rewrite it against the runtime-width accessors (materialize_row/assign_row/row_popcount/ for_each_row_position over MonomialLike, detail::OperatorIndex and detail::SparseRowStore) and give Utilities.h -- which calls materialize_row and had been getting MonomialLike transitively through the now-slimmed TypeAliases.h -- a direct include. AGENTS.md's row-store-seam paragraph named the old home; point it at RowAccess.h instead. - #288 (main)'s combined_recompute_equivalence.cpp cases call load_case_data/build_simulator/generator_of/make_lazy_fold/make_fold_cache/ fold_popcount/accumulate_cos_lazy with an explicit <kNumModes> template argument, matching main's NTTP-templated signatures at the time. Drop the explicit arguments to match this branch's runtime-width ones, as the rest of the file's call sites already do. 632 CTest cases (638 with sparse-row variants) and the Python suite pass. Assisted-by: ClaudeCode:claude-sonnet-5

…226 No functional change. Mechanical, no-functional-change lint cleanup across 19 files: SonarCloud flagged 187 Tier-1 findings on PR #226 (default lambda captures, redundant explicit types, redundant lambda return types, push_back -> emplace_back, non-const-ref params/locals made const, using enum, std::thread -> std::jthread, structured bindings, redundant inline on constexpr functions, and reused abbreviated-template parameters given an explicit name). Three findings were deliberately left as-is: - PartitionGroup.h:212 (S6004): folding the declaration into the if-init would end its lifetime while its address is still read afterward. - CodesAlgebra.h:257 (S6188): raw-pointer+capacity -> std::span needs a call-site survey; deferred rather than guessed. - Bitset.h:148 (S3490): the hand-written empty-body Storage() constructor is user-provided, which is what makes Bitset const-default-constructible; a defaulted one is not user-provided and breaks `const Bitset zero;` in the test suite despite being runtime-identical. Left unchanged. Verified: full uv sync rebuild (0 errors), ctest 639/639 (dense + sparse-row variants), uv run pytest 625 passed / 8 skipped (MPI, needs --with-mpi). Assisted-by: ClaudeCode:claude-sonnet-5

`Fn &&func` is a template-deduced forwarding reference, but the returned closure captured it with `std::move(func)`. For an lvalue argument, `Fn` deduces to a reference type, so `Fn&&` collapses to an lvalue reference and `std::move` unconditionally treats it as movable regardless of what the caller actually passed in. Both current call sites (`ev_fn`, `ev_and_grad_fn`) happen to be `static inline const auto` member lambdas, so `Fn` deduces const and the `std::move` silently degrades to a copy today -- masking the bug. Any future caller passing a mutable lvalue functor would have it silently gutted. `std::forward<Fn>(func)` respects the value category the caller actually passed, which is what a forwarding reference is for. Found via SonarCloud analysis of PR #226 (rule cpp:S5417). Assisted-by: ClaudeCode:claude-sonnet-5

…25, S7172, S6188) - S5817: mark five genuinely non-mutating member functions const (MonomialPropagator::evolve_operator_with_recompute_, GraphSink::process_reserve, FusedSink::finalize, PartitionGroup::report_placement_, PartitionGroup::comm_for_). DenseQueryKeys::read_record in Common.h is left non-const: it decodes into keys_[slot] via a non-const out-param, so const would not compile — a Sonar false positive. - S5425: MPOperator::for_each_term now forwards its callable (rows.for_each(std::forward<Fn>(fn))); safe because with_store() invokes its lambda exactly once. The MonomialPropagator.cpp:932 instance was already resolved by the earlier S5417 fix. - S7172: replace optional truthiness checks with .has_value() in TermProduct.h and MPOperator.h. - S6188: replace CodesAlgebra.h's sparse_toggle(RowMode*, size_t) pair with a single std::span<RowMode> parameter, after surveying all five call sites (one hot per-term path, four tests) to confirm every one already passes a pointer+size where size matches the container exactly — a zero-cost signature change. Verified with a full uv sync rebuild (0 errors), 639/639 ctest, 625 passed/8 skipped-MPI pytest, and clang-format clean. Assisted-by: ClaudeCode:claude-sonnet-5

S2807 (hidden friend for operator==) and S5025 (smart-pointer new/delete) on Bitset.h, following the same one-rule-one-file sonar.issue.ignore.multicriteria pattern already used for the other documented exceptions in this file. Rationale for each lives next to its entry: - S5025's new[]/delete[] are the heap arm of the hand-rolled Storage union, matched 1:1 across the five special members that touch it — exactly the code a naive =default rewrite already broke once (the S3490 regression fixed alongside the Tier-1 cleanup). - S2807 is a no-op here since every Bitset constructor is explicit; deferred to the same pass as the S1448 class-split finding on this file rather than one operator at a time. No functional change. Assisted-by: ClaudeCode:claude-sonnet-5

Assisted-by: ClaudeCode:claude-sonnet-5

A SparseRowStore codes word is two bits per slot, so a store sized from a cutoff bound only ever sets 12 of 64 bits at cutoff 6 and 16 at cutoff 8 -- the two shipping models. Pick the narrowest of uint16_t/uint32_t/uint64_t that holds 2 * slots_per_row, over three arrays bound by with_codes(), the same way OperatorIndex already picks its row width. Worth -30% and -25% of operator_terms_bytes on the benchmark Hubbard model (20 -> 14 and 24 -> 18 live bytes per row): at the 256-mode crossover the two backends' row arrays are now byte-for-byte equal. Not a time cost either -- the dispatch and the narrowing casts are +2.2% user instructions, but cycles:u falls 1-2%, because four times as many rows fit each line of the array the cutoff walk reads. Only the array narrows; every reader still sees a zero-extended CodesT, so CodesAlgebra.h, sparse_row_hash and SparseRow are untouched and both baselines stay byte-identical. Assisted-by: ClaudeCode:claude-opus-5

A batch's retained keys -- the ones the deferred self-miss list reads after the slots have been refilled -- were a MonomialList. A Bitset is sized for the widest inline width whatever its own is, so that carried 72 bytes per key where a 128-bit monomial needs 16, and one key is retained per term a layer inserts. Store the words flat at the batch's own width instead, as the support-form batch already did. Worth -1.2% user instructions and cycles on the benchmark Hubbard model, pinned single-threaded; the gain roughly triples at a looser lower_atol, which inserts fewer terms per layer. retained() now hands back a scratch view, so at most one retained key may be read at a time. Both readers satisfy that, and the support form's retained() already had the same contract. Assisted-by: ClaudeCode:claude-opus-5

The header block still described MonomialPropagator<NumModes> and an explicit class-template instantiation. There is no instantiation to force member emission with any more -- the reachable set is whatever main() calls. Assisted-by: ClaudeCode:claude-opus-5

sonarqubecloud · 2026-08-28T16:55:07Z

Quality Gate passed

Issues
61 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

robertodr · 2026-08-29T18:30:31Z

closing: it's been unpacked into a stack of changes instead, for easier review

github-actions Bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file python cpp ci tools labels Aug 13, 2026

This comment was marked as outdated.

Sign in to view