refactor!: drop non-type template parameter for number of modes

[robertodr]

Summary

Changes

Checklist

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

AI/LLM disclosure

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

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]

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

[robertodr]

benchmarks against main for the Hubbard model

Read rows <num_sites>_<cutoff>_<lower_atol>

Time

Operation	main	refactor-drop-nttp
32_6_6	52.871 s	61.417 s
32_8_5	15.661 s	16.081 s
64_6_6	61.750 s	67.608 s
64_8_5	16.498 s	17.360 s
124_6_6	81.586 s	80.307 s
124_8_5	19.680 s	20.404 s

Memory (peak RSS)

Operation	main	refactor-drop-nttp
32_6_6	18617.14 MiB	19386.70 MiB
32_8_5	6700.80 MiB	8945.71 MiB
64_6_6	18929.92 MiB	19462.99 MiB
64_8_5	6766.88 MiB	9028.16 MiB
124_6_6	19570.04 MiB	19675.12 MiB
124_8_5	8200.38 MiB	8295.32 MiB

[robertodr]

🤖 AI text below 🤖

memory_bytes() charges each spilled row sizeof(Bitset) + sizeof(size_t) + 24 but never adds what the Bitset owns off-object. AGENTS.md (this branch) states the rule: "memory accounting over a container of monomials must add Bitset::heap_bytes() per element; sizeof(Bitset) per element looks right until someone runs a wide system."

That is exactly the regime this branch adds: at 288 storage modes a monomial is 9 words, past Bitset::kInlineWords, so every overflow_ entry heap-allocates 72 bytes that operator_memory_breakdown()["operator_terms_bytes"] silently drops. monomial_map_bytes() right next door already does this correctly for init_op_map. OperatorIndex::memory_bytes() has the same gap.

Suggested change

	total += overflow_.size() * (sizeof(value_type) + sizeof(size_t) + 24);
	[[nodiscard]] auto memory_bytes() const -> size_t {
	size_t total = modes_.capacity() * sizeof(ModeT);
	total += codes_.capacity() * sizeof(CodesT);
	for (const auto &kv : overflow_) {
	total += sizeof(value_type) + sizeof(size_t) + 24 + kv.second.heap_bytes();
	}
	return total;
	}

[robertodr]

🤖 AI text below 🤖

src/monoprop/bindings/binder.h no longer exists — commit 70d839c ("remove binder.h") folded it into src/monoprop/bindings/bindings.cpp. Line 214 ("Add Python bindings in src/monoprop/bindings/binder.h") is stale for the same reason.

The repo's own Documentation Maintenance Policy in this file says to update AGENTS.md in the same change and to fix or remove any section that no longer reflects the codebase.

[robertodr]

🤖 AI text below 🤖

configure() clears lanes_, keys_, escapes_, retained_lanes_ and retained_, but not retained_bases_ or retained_escapes_.

retain() derives its handle from retained_.size() while pushing the matching base onto retained_bases_. If retained_ is cleared and retained_bases_ is not, the two arrays desynchronise: handle 0 then reads retained_bases_[0], a stale base into a retained_lanes_ that was just emptied, and retained() indexes out of bounds. retained_escapes_ has the same pairing problem plus unbounded growth.

Today this is latent — the engine's keys_ is constructed fresh per build_layer and the thread_local probe batch in Resolve.h never calls retain() — but the invariant is one configure() call away from being live.

Suggested change

	num_bits_ = num_bits;
	if (capacity_ != capacity || num_bits_ != num_bits) {
	lanes_.clear();
	keys_.clear();
	escapes_.clear();
	retained_lanes_.clear();
	retained_bases_.clear();
	retained_escapes_.clear();
	retained_.clear();
	capacity_ = capacity;
	num_bits_ = num_bits;
	}

[robertodr]

🤖 AI text below 🤖

The dense arm below sizes its rows through packed_inline_width_(), which special-cases Schrödinger (if (schrodinger_) return kDefault;). The sparse arm bypasses that rule entirely and sizes from cutoff_ alone.

In Schrödinger the initial term set is every fully paired monomial with up to max_pairs = ceil(sc/2) occupied modes, and sc is schrodinger_cutoff — an independent user knob, not cutoff. Whenever ceil(sc/2) > cutoff_ (e.g. cutoff=1, or any explicitly large schrodinger_cutoff), every one of those initial rows exceeds slots_per_row_ and lands in the overflow_ unordered_map — a heap Bitset plus a map lookup on the per-term path, for what should be the common case.

The two backends should read the row width from one place; packed_inline_width_() already is that place for the dense store.

[robertodr]

🤖 AI text below 🤖

This block duplicates the if(monoprop_ENABLE_CXX_UNIT_TESTS) enable_testing() / include(CTest) endif() that already sits at lines 152-158, immediately before add_subdirectory(cpp) (added on main by #224). Both are now in the file, each carrying a comment that claims to be the reason the call lives where it does — and they give different reasons.

Harmless at build time (CTestTargets.cmake guards its own targets with the CTEST_TARGETS_ADDED global property), but the next reader has to work out which comment is true. Keep one.

[robertodr]

🤖 AI text below 🤖

Related cleanup this PR misses (line 89, not reachable as an inline anchor):

[tool.uv]
cache-keys = [
  ...
  { file = "src/monoprop/bindings/bindings.cpp.in" },   # <- deleted by this PR
]

This PR deletes src/monoprop/bindings/bindings.cpp.in (it became src/monoprop/bindings/bindings.cpp), but the cache key still points at the removed template, and no remaining entry covers src/monoprop/bindings/** — cpp/include/**, cpp/monoprop/**, **/CMakeLists.txt and cmake/** all miss it.

So editing the nanobind bindings no longer invalidates uv's build cache: a later uv sync can hand back a wheel built from the previous bindings.cpp. Suggest { file = "src/monoprop/bindings/*.cpp" }.

[robertodr]

🤖 AI text below 🤖

This comment is written against MonomialPropagator.inl, but the same PR renames that file to cpp/monoprop/detail/monomial_propagator/MonomialPropagator.cpp. The include is now from the .cpp, and the ordering contract the comment describes (include after MonomialPropagator's definition, because the member bodies are ordinary functions needing the complete type) is what a reader is meant to be able to check — against a filename that no longer exists.

[robertodr]

🤖 AI text below 🤖

logical_num_modes has no default but is declared after five defaulted arguments (comm, schrodinger_cutoff, lower_atol, upper_atol, cutoff_type, basis_change). nanobind builds the signature from these in order, so the docstring/stub signature is (..., basis_change=None, logical_num_modes, basis="majorana", ...) — not valid Python, and any positional call has to pass all six preceding arguments.

src/monoprop/monomial_propagator.py only ever calls this with keywords, so nothing breaks today, but moving logical_num_modes up next to initial_state (matching the C++ constructor, where it already sits third) would make the exposed signature honest.

[robertodr]

🤖 AI text below 🤖

"Rows in index order" is not what this does — RowHashTable::for_each_slot walks the slot array, so rows come out in hash/probe order, not ascending row index. OperatorIndex::for_each and MPOperator::for_each_term both describe it correctly as "table order" / "slot order", and the class comment 300 lines up is explicit that the two stores deliberately differ in that order.

Given this order is load-bearing (it fixes the user-visible evolved-term order and the FP accumulation order), a comment that says "index order" is the kind that gets believed.

[robertodr]

🤖 AI text below 🤖

"Grow-only and never cleared between layers ... the buffer holds the largest layer's worth until the thread exits" is true of the thread_local batch in Resolve.h, but not of the other user of this class: LayerBuildEngine::keys_ is a plain member of an engine that is constructed fresh inside build_layer, i.e. once per layer. Its configure() therefore always sees a default-constructed object and ensure(kResolveBatch) re-builds 64 elements every layer — the allocation this comment says was measured away.

(It has to be per-layer, because retain() handles index into retained_, which would otherwise grow without bound across layers — so the fix is the comment, not the lifetime.) Resolve.h line ~110 repeats the claim ("the same trade nz and keys_ already make").

[robertodr]

🤖 AI text below 🤖

The store is created once here with a row width derived from cutoff_, but cutoff_ is mutable for the propagator's lifetime: update_cutoff() (MonomialPropagator.h) calls regenerate_cutoff_fn_() and nothing else, so the store keeps slots_per_row_ = slots_for_bound(old_cutoff).

Raising the cutoff then pushes every row wider than the old bound into overflow_ — a heap Bitset plus an unordered_map lookup on the per-term path, for the typical row rather than the ~0.07% the spill was sized for. Still correct (the spill is lossless), but it is a silent order-of-magnitude cliff with no diagnostic; partition_equivalence_tests.cpp exercises update_cutoff() today, just below the sparse crossover where it cannot show up.

Worth either re-creating the store on a cutoff change or documenting that slots_per_row_ is a construction-time bound.

[robertodr]

🤖 AI text below 🤖

Note on memory_bytes() just above (line 189-193, outside the diff so not directly anchorable):

total += overflow_.size() * (sizeof(value_type) + sizeof(size_t) + 24);

value_type is now a runtime-width Bitset, and above Bitset::kInlineWords (8 words / 250 modes) each spilled row owns a heap array this never counts. The line was correct when value_type was Monomial<NumModes>, a fixed-size type with no allocation — de-templating Bitset is what invalidated it. SparseRowStore::memory_bytes() copies the same shape.

AGENTS.md (added in this PR) states the rule: "memory accounting over a container of monomials must add Bitset::heap_bytes() per element; sizeof(Bitset) per element looks right until someone runs a wide system." And 288 storage modes (9 words) is precisely the regime tests/test_wide_system.py adds here. monomial_map_bytes() in MPOperator.h already does it correctly.

[Panadestein]

Nice that the sparse backend gets a per-case second run here!

It looks like this env var never reaches the MPI variants, though. The register_variant at L253–262 sets only the OMPI root vars, just test-sparse-rows runs pytest -m "not mpi", and sparse_wire_tests.cpp exercises the codec in-process with no communicator. So the sparse backend is currently covered serially but never across ranks.

That's a bigger gap than it looks, because the sparse form isn't just local storage; it changes the wire format. query_payload_words_for gives a different stride per backend, and the escape tail, kOverflowLane and append_escape_tail have no dense counterpart, so that whole path only ever runs single-rank. It's also the backend that ships for wide systems, which is exactly where MPI gets used.

Could we add monoprop_ROW_STORE=sparse MPI variants, even just at 2 ranks? Happy for it to be a follow-up if there's a reason to defer.

[Panadestein]

The comment here very honest, and I agree it's the right call for the
inner loop. One consequence worth spelling out, since the comment frames it as a call-site obligation: on main this was template <size_t NumBits> class Bitset, so a width mismatch was a compile error. Now it's assert only, which compiles out under NDEBUG, so in Release a mismatch is a silent OOB read of the inline std::array<word_type, 8>, and an OOB read of a heap allocation once the operand has spilled. No crash, no diagnostic, just a wrong number.

The same applies to L282, L344, L361, L378 and L548–549.

I couldn't find a call site that actually violates this, so this isn't a bug report, just an observation. Would a width assert at those construction boundaries be worth it?