tech-debt-backlog: §3.11, §5.10–§5.13, §5.15, §5.2 — concept_gram_mps polish

[jascal]

Summary

Cluster of small concept_gram_mps fixes from the 2026-05-01 PR #48 self-review and Sonnet 4.6's PR #45 review. Picks 7 contiguous backlog items that all touch q_orca/compiler/concept_gram_mps.py and its tests so the diff stays cohesive.

Code changes (q_orca/compiler/concept_gram_mps.py)

• §3.11 — Folded the angle-matrix build into the per-call-site loop and wrapped float(b.value) in try/except that re-raises as MpsGramConfigurationError naming the call site, action, argument index, and offending value. Defensive guard for programmatically-built QMachineDefs; the parser only ships int/float literals into BoundArg.value today.
• §5.12 + §5.13 — Refactored the Mult branch of _parse_linear_combination through a new _as_numeric_const helper that recognizes both bare Constant and UnaryOp(USub|UAdd, Constant) shapes. As a side effect this also fixes the canonical -2*a form (previously silently rejected — the AST is BinOp(Mult, UnaryOp(USub, Constant), Name), not BinOp(Mult, Constant(-2), Name)). The two-constants case (2*3) now raises with a clearer "no parameter reference" message.

Tests (tests/test_compiler.py, TestComputeConceptGramMps)

5 new tests:

• test_unrecognized_angle_expression_bare_literal_raises (§5.10)
• test_unrecognized_angle_expression_power_raises (§5.10)
• test_inverse_form_linear_combination_matches_prep_form (§5.11)
• test_constant_times_negated_param_accepted (§5.12)
• test_constant_times_constant_error_mentions_no_parameter (§5.13)
• test_call_site_non_numeric_bound_argument_raises (§3.11)

Docs / spec

• §5.15 — openspec/specs/language/spec.md: replaced the misleading symbolic example qc.ry(a_value + b_value, 1) with the actual fully-evaluated-float emission qc.ry(-0.85, 2) (verified against the live compiler output for the canonical example).
• §5.2 — docs/compute-needs.md: bumped "All 15 example machines" → "All 19". The README references had already drifted forward via interim PRs; only this doc remained stale.

Backlog updates (openspec/changes/tech-debt-backlog/tasks.md)

Marked §3.11, §5.2, §5.10, §5.11, §5.12, §5.13, §5.15 done with the standard task-body completion notes.

Test plan

• pytest -x -q — 852 passed, 6 skipped
• ruff check q_orca/compiler/concept_gram_mps.py tests/test_compiler.py — no new violations on touched code (one pre-existing unused numpy import in an unrelated test was left alone to keep the diff focused)

🤖 Generated with Claude Code

[jascal]

Code Review — Claude Sonnet 4.6

Reviewing 7 backlog items rolled into one cohesive concept_gram_mps polish PR. Verified locally on tech-debt-backlog-mps-gram-polish.

Verification

• Tests: .venv/bin/pytest --tb=short -q → 852 passed, 6 skipped ✅
• Lint: .venv/bin/ruff check . → 2 errors, both pre-existing in files this PR does not touch:
  • tests/test_compiler.py:1611 — unused numpy import (PR author called this out in the description)
  • tests/test_examples.py:319 — unused cr_min variable (introduced by #57, also unrelated)
  • Touched files (q_orca/compiler/concept_gram_mps.py, the new test functions, the spec/doc files) are clean. Not a blocker.

Correctness

§5.12 + §5.13 — Mult branch refactor (concept_gram_mps.py:161-212): The new _as_numeric_const helper is a clean rewrite. It correctly handles:

• bare Constant ✅
• UnaryOp(USub, Constant) (the canonical -2 shape — important; the previous AST-shape match silently rejected -2*a, as the PR notes) ✅
• UnaryOp(UAdd, Constant) (the rare +2 shape) ✅
• recursive nesting (--2 → 2) ✅

A nice bonus side effect worth calling out: because the new branches recurse via walk(node.right, sign * left_val) instead of only matching Name nodes, expressions like 2 * (a + b) now distribute correctly into {a: 2, b: 2}. That wasn't promised but it's a strict improvement over the old Constant * Name / Name * Constant shape match and is consistent with the helper's contract ("linear combination").

§3.11 — float-coercion guard (concept_gram_mps.py:614-636): Folding the angle-matrix build into the per-call-site loop is the right structural call — it gives the float coercion access to site_idx and arg_idx for the diagnostic without a second pass. The error message is well-formed (machine name, action label, call site index, arg index, offending repr, underlying exception). Catching both TypeError and ValueError is correct (float("not a number") raises ValueError; float([1]) raises TypeError).

§5.10 — bare-literal sad path: The _parse_linear_combination helper already raised on bare ast.Constant at line 213-217; the new test pins that the call-chain surfaces it as unrecognized_angle_expression. Good gap-fill.

The choice in test_unrecognized_angle_expression_power_raises to use a**2 rather than the spec's literal a^2 is well-defended in both the test docstring and the backlog note: a^2 parses as BitXor(Name(a), Constant(2)), which is also unsupported but for a different surface reason. ** is the more user-likely shape and lands in the same unsupported expression node branch.

§5.11 — inverse-form coverage: test_inverse_form_linear_combination_matches_prep_form is well-designed — it asserts |gram_inverse| == |gram_prep| rather than equality, correctly avoiding the global-phase-difference trap that catches naive comparisons of inverse-form Grams. The triple choice (mix of positive, negative, and small magnitudes) gives reasonable signal.

§5.15 — spec example: I verified the bound triple a=0.0, b=-0.5, c=-0.35 against examples/larql-polysemantic-hierarchical.q.orca.md:188 and larql-animals-hierarchy.q.orca.md:47. The expected emission qc.ry(-0.85, 2) (= b + c evaluated) is internally consistent. Good catch on the misleading symbolic example.

§5.2 — example count: Confirmed ls examples/*.q.orca.md | wc -l = 19, and README.md:12 already reads "All 19 bundled example machines". Only docs/compute-needs.md:155 was stale; bumping it to 19 brings everything into agreement.

Test coverage

• 6 new tests (the PR description says "5" but lists 6 — minor description nit, not a blocker).
• Sad paths covered: bare literal, power, all-constants product, non-numeric bound arg ✅
• Happy paths: 2*-a ≡ -2*a equivalence, inverse-form ≡ prep-form magnitude ✅
• The parser-bypass pattern (mutating action.effect post-parse) is consistent with the existing test_rz_unrecognized_angle_expression_raises test in the same class — appropriate use for defense-in-depth helper-level guards that the user-facing validator otherwise short-circuits.

Minor observations (non-blocking)

1. _as_numeric_const and bool: isinstance(True, int) is True in Python, so True * a would now be accepted as 1 * a. This is a pre-existing behavior of the original Constant * Name match (it also accepted bools), so the refactor is neutral on this. Not worth fixing.

2. The "product of two numeric constants" branch (line 199-203): This is reachable from inside a recursive walk too (e.g., (2*3) * a → walk(BinOp(Mult, Constant(2), Constant(3)), 2.0) after the outer Mult resolves the right-side Name(a) first → wait, no, it depends on which side is the constant). Worth tracing once: for (2*3) * a, node.left = BinOp(Mult, 2, 3) → _as_numeric_const returns None (it doesn't recurse into BinOp), node.right = Name(a) → None. Falls through to "product of two non-constant terms". So the all-numeric guard only fires for direct const * const shapes, which matches §5.13's intent. Fine as-is.

3. Test description in PR body says "5 new tests" but lists 6. Cosmetic.

What looks good

• Cohesive scope: every change touches concept_gram_mps.py or its tests/spec, justifying the bundled PR.
• Each fix has both an implementation change and a targeted regression test.
• Backlog items are marked done in tasks.md with completion notes that capture the "why" (e.g., the ** vs ^ decision, the bonus -2*a fix that came along with 2*-a, the audit-at-fix-time finding that two of the three references in §5.2 had already been fixed).
• The MpsGramConfigurationError for non-numeric bound args is much more debuggable than a bare ValueError("could not convert string to float: 'not a number'") would be.
• The Mult-branch refactor is a strict improvement — same surface contract, fewer dead/silently-wrong code paths.

Verdict

Approve / ready to merge (modulo the two pre-existing ruff warnings in unrelated files, which are out of scope here). Solid, focused tech-debt cleanup with good test discipline.

This review was posted automatically by Claude Sonnet 4.6.

[jascal]

Code Review — Claude Sonnet 4.6

Nice cohesive cleanup — six items that all touch the same module and share a clear narrative around concept_gram_mps's parser and call-site coercion. Tests and lint look clean on my end (852 passed, 6 skipped; the two pre-existing ruff findings on tests/test_compiler.py:1611 and tests/test_examples.py:319 are both already on main and not introduced here).

What looks good

• §3.11 contextful coercion error. Folding the angle-matrix build into the per-call-site loop (q_orca/compiler/concept_gram_mps.py:614-636) is the right move — the arity guard and the float coercion now share one iteration context, and the new error names machine, action, call site #idx, argument #idx, the offending repr, and chains the underlying exception via from e. That's exactly the diagnostic a programmatically-built-machine consumer needs.
• §5.12 + §5.13 helper refactor. The _as_numeric_const helper (concept_gram_mps.py:161-173) is a clean, recursive way to unify Constant, UnaryOp(USub, Constant), and UnaryOp(UAdd, Constant). The Mult branch is now strictly more general and easier to reason about: numeric on either side → recurse on the other operand with the folded sign. The all-constants short-circuit (concept_gram_mps.py:199-203) lands before the legacy "two non-constant terms" fall-through, which preserves that diagnostic for the genuinely non-numeric case.
• §5.11 inverse-form pinning test. test_inverse_form_linear_combination_matches_prep_form is the right shape — it builds equivalent prep- and inverse-form machines on the same triples and asserts |gram_inverse| == |gram_prep| to 1e-12. Catches both _parse_linear_combination's is_inverse path and the contraction code in one go.
• §5.15 spec accuracy. Replacing the symbolic qc.ry(a_value + b_value, 1) example with the actually-emitted qc.ry(-0.85, 2) (verified against the live compiler output) closes a real doc/code gap.
• Backlog notes. The completion notes on each tasks.md item are unusually thorough — calling out the ** vs ^ AST difference (§5.10 note) and the parser-template-validator interaction (§5.12 note) are both useful for future readers.

Questions / suggestions

1. Quietly broader parse surface — worth a thought. The new _as_numeric_const + walk recursion accepts more than the description claims. I confirmed locally on the branch:

   • 2 * (a + b) → {'a': 2.0, 'b': 2.0} (was rejected by the old Constant * Name shape match)
   • 2 * a * 3 → {'a': 6.0} (was rejected; outer Mult had a BinOp on the left)
   • -2 * (a - b) → {'a': -2.0, 'b': 2.0}

   These are arguably correct behaviors for a "linear combination" parser (they remain linear), but the PR description / spec language only advertises folding 2*-a and -2*a. Two options:

   • Document + test the expansion. Add a one-liner test pinning 2 * (a + b) and update the helper docstring's enumeration in _parse_linear_combination (concept_gram_mps.py:142-153) to say "any product of a numeric constant and a linear-combination subexpression."
   • Or restrict back. If you only meant to fix the ±2 * ±a shapes, gate the recursive call on isinstance(other, (ast.Name, ast.UnaryOp)) to keep the surface tight.

   I'd lean toward (1) — the broader behavior is mathematically sound and the recursion is cleaner. But it's a meaningful semantic change that ought to be either advertised or fenced.

2. bool slips through (int, float). isinstance(True, (int, float)) is True in Python, so True * a would now fold to {'a': 1.0}. This matches the old behavior (the old branch had the same isinstance check), so no regression — but if you're already touching this code, swapping to type(n.value) in (int, float) would close the door cleanly. Total nit; ignore if you'd rather keep the diff scoped.

3. §3.11 test relies on BoundArg mutability. test_call_site_non_numeric_bound_argument_raises does machine.transitions[1].bound_arguments[1].value = "not a number" to bypass parser validation. Works today; would silently break if BoundArg ever becomes a frozen dataclass. Probably not worth changing now, but worth a comment noting the dependency in case someone hardens AST node immutability later.

4. Pre-existing unused numpy import inside TestComputeConceptGramMps. The PR description says "an unrelated test was left alone," but tests/test_compiler.py:1611 is actually inside TestComputeConceptGramMps (test_rz_phase_invariance_under_global_phase) — not unrelated to this PR's class. Trivial to drop while you're here. (But ruff still passes on the touched code, so this is purely a description nit.)

Verdict

LGTM with one judgment call to make: decide whether the quietly-expanded parse surface in #1 should be advertised + tested, or fenced back. Everything else is style-level. Tests pass, lint is clean on touched code, the spec update is verified, and the backlog tracking is meticulous.

This review was posted automatically by Claude Sonnet 4.6.

[jascal]

Code Review — Claude Sonnet 4.6

Verdict

LGTM. Cohesive cluster of small concept_gram_mps fixes — every change is well-scoped, every behavioral change has a pinning test, and the bundling is justified by all touching the same file/test class.

What looks good

• _as_numeric_const helper is the right abstraction. Recognizing both bare Constant and UnaryOp(USub|UAdd, Constant) shapes in one place lets both Mult operands be probed symmetrically (q_orca/compiler/concept_gram_mps.py:161-173). The recursion through UnaryOp chains means even pathological inputs like 2*--a (AST: Mult(Constant(2), UnaryOp(USub, UnaryOp(USub, Name)))) wouldn't blow up — the inner call returns None once it hits the Name, and the Mult branch falls through to walk(node.right, sign * 2.0), which itself unwraps the USubs. Clean.
• Side-effect fix for -2*a is a real bonus. The §5.12 task only mentioned 2*-a, but the canonical user-written -2*a form (which parses as BinOp(Mult, UnaryOp(USub, Constant), Name)) was rejected by the same fall-through. Folding both into the helper closes a wider gap than the task requested. The test (test_constant_times_negated_param_accepted) feeds both shapes through the same parser-bypass and asserts identical Grams — the right way to pin equivalence.
• §5.13 error message refinement is precise. Lifting the all-constants check above the "non-constant terms" raise (q_orca/compiler/concept_gram_mps.py:199-203) means the misleading wording now only fires on the case it was actually written for. The test (test_constant_times_constant_error_mentions_no_parameter) asserts both the new wording lands AND the old wording is absent — that second assertion is what makes the test load-bearing rather than redundant.
• §3.11 contextful error is informative. Naming the machine, action, call-site index, argument index, and offending repr (q_orca/compiler/concept_gram_mps.py:621-636) gives the user everything they need to localize the problem in a programmatically-built QMachineDef. Catching (TypeError, ValueError) rather than just ValueError is a sensible widening (float(None) raises TypeError).
• Test scoping discipline. Every helper-level test that exercises an angle expression the user-facing parser would reject upstream uses the parser-bypass pattern (action.effect = ... post-parse). The tests' docstrings call this out explicitly, so a future reader won't be confused into thinking the parser silently accepts 2*-a end-to-end. Good defense-in-depth framing.
• §5.11 inverse-vs-prep equivalence is the right correctness invariant. |G_inverse| == |G_prep| to 1e-12 with three distinct angle triples is a strong signal — it pins both the linear-combination parser and the staircase contraction, and would catch sign-flip regressions in either.

Lint / tests

• .venv/bin/pytest --tb=short -q → 852 passed, 6 skipped ✓ (matches the PR description).
• .venv/bin/ruff check . → 2 errors (F401 unused numpy in tests/test_compiler.py:1611, F841 unused cr_min in tests/test_examples.py:319). Both reproduce on main — pre-existing, not introduced here. This PR is clean on its touched files.

Concerns / suggestions (mostly non-blocking)

1. §3.11 changes the error-priority order between arity and coercion. Pre-PR: ALL call sites' arity was checked first, THEN float coercion ran for any call site (q_orca/compiler/concept_gram_mps.py around the old for t in call_sites: if len(...) != n_params: raise). Post-PR: arity and coercion are interleaved per call site. So if call site #0 has good arity but a non-numeric value AND call site #1 has wrong arity, the old code raised an arity error for #1; the new code raises a coercion error for #0. Both indicate misconfiguration so the behavior change is benign, but it's a real semantic shift that the task description doesn't mention. Fine to ship as-is — just flagging in case the priority ordering matters to a downstream caller catching specific error shapes.

2. §5.15 spec example value claim is unverified by an automated test. The new spec text says qc.ry(-0.85, 2) for a=0.0, b=-0.5, c=-0.35. The arithmetic checks out (b + c = -0.85, qubit index 2 is the third Ry in the prep-form staircase), and the PR description claims it was verified against the live qiskit compiler output. Worth considering: if the spec's value drifts from the actual emission later (e.g., a sign convention flip in compile_to_qiskit), nothing in the test suite would catch the spec divergence. A small test that grep-asserts qc.ry(-0.85, 2) actually appears in the qiskit emission for the canonical example would close that loop. Optional — it's a doc-only assertion.

3. §5.2 leaves the original task body's 15 → 16 framing intact while the completion note documents the actual fix-time count (19). A future reader has to read both halves to understand. Minor — and consistent with how other tasks in the same backlog are annotated.

4. _as_numeric_const could eventually live alongside evaluate_angle. Both are angle-expression unwrappers and share the "strip leading USub/UAdd" pattern; the angle.py side handles a string and the compiler side handles an AST. Not worth refactoring now — the AST view is small and the duplication is shallow — but if a third caller wants the same logic, that'd be the time to consolidate.

Security / performance

No concerns. All changes are pure-Python, no new external inputs, no I/O. The Mult-branch refactor adds at most two _as_numeric_const calls per Mult node, both bounded by AST depth — same asymptotic cost as the original.

Bottom line

Ship it. The bundled scope is justified, every fix is pinned, the §5.12/§5.13 refactor is genuinely cleaner than what it replaces, and the §3.11 contextual error is the right shape of diagnostic for a programmatically-built machine. Two of the four "interim PR already addressed" notes (§5.2 README, §5.15 docstring) save real effort by not re-touching settled code.

This review was posted automatically by Claude Sonnet 4.6.

[jascal]

Code Review — Claude Sonnet 4.6

Summary

Tight, cohesive cluster of fixes around concept_gram_mps. Tests pass (852 passed, 6 skipped) and ruff is clean on touched files (the two pre-existing F401/F841 warnings in tests/test_compiler.py:1611 and tests/test_examples.py:319 are unrelated to this PR and the PR description correctly flags the first one).

What looks good

• _as_numeric_const refactor (§5.12, §5.13) is the right abstraction: it normalizes both bare Constant and UnaryOp(USub|UAdd, Constant) shapes, and as a bonus catches the canonical -2*a AST shape (BinOp(Mult, UnaryOp(USub, Constant), Name)) that previously fell through the same branch. The dual-shape coverage in test_constant_times_negated_param_accepted (asserting 2*-a and -2*a produce identical Grams) is exactly the right pin.
• §3.11 contextful float-coercion guard is a textbook defense-in-depth: the error names machine, action, call-site index, argument index, and offending repr (q_orca/compiler/concept_gram_mps.py:629-635). Folding the angle-matrix build into the existing per-call-site loop (rather than keeping the comprehension) was the right call — keeps the arity guard and the float coercion in one iteration context.
• §5.11 inverse-form symmetry test is a strong invariant: comparing |gram_inverse| to |gram_prep| on three triples directly pins the helper's is_inverse path against silent regressions.
• §5.15 spec example update (qc.ry(-0.85, 2) instead of the misleading symbolic qc.ry(a_value + b_value, 1)) is verified against live compiler output, which is exactly the kind of grounding spec examples need.

Correctness

• The Mult-branch refactor preserves all previously-accepted shapes (Constant * Name, Name * Constant) and adds the four new shapes (Constant * UnaryOp(USub, Name), UnaryOp(USub, Constant) * Name, etc.) without behavioral surprises. Zero coefficients (0 * a) still flow through as add(name, 0.0), matching the prior path.
• The _as_numeric_const recursion on UnaryOp chains is unbounded in principle (e.g., --+-a), but Python's AST produces these naturally and the evaluator already accepts -(-x) semantics via the existing walk USub branch — so the recursive normalization is consistent.
• _NonLinearExpr("product of two numeric constants has no parameter reference") propagates through _parse_angle_expr to MpsGramConfigurationError(kind="unrecognized_angle_expression"). The kind tag is unchanged, so existing callers that match on kind keep working.

Test coverage

Six new tests cover the right cases:

• bare-literal sad path (2.5 in a Ry slot) ✓
• power sad path (a**2) ✓
• inverse-form happy path (cross-coupled angle, magnitude match against prep) ✓
• 2*-a / -2*a happy paths (parser-bypass via effect mutation) ✓
• 2*3 rejection with new "no parameter reference" wording, and a negative assertion that "non-constant" no longer appears ✓
• non-numeric BoundArg.value produces the contextful error ✓

The test docstrings are unusually thorough — they cite the source spec and explain why each assertion exists. That's exactly the right shape for backlog-driven coverage.

Minor observations (not blockers)

• The error kind for "product of two numeric constants" is still unrecognized_angle_expression, which is technically correct but a notch off from the new wording's spirit ("no parameter reference"). Worth considering a distinct kind if other callers ever start branching on this case — for now the behavior is preserved and nothing in the test suite distinguishes them.
• The §3.11 try/except catches (TypeError, ValueError) — appropriate, since float("string") raises ValueError and float(None) raises TypeError. Both are tested only via the string case; the None path is implicitly covered by the same code branch but not asserted. Not worth adding a second test.
• The _as_numeric_const helper is defined inside _parse_linear_combination, which keeps it local to its caller — fine, though if a third Mult-shape needs it later, a module-level helper would be easier to extend.

Verdict

Ship it. Implementation is clean, tests are precise, spec example is verified. The cluster size is well-chosen — all touched code lives in one module and one test class, so the diff stays cohesive.

This review was posted automatically by Claude Sonnet 4.6.