Rank gate on tensor_inv wrapper ctor (#292) - #294
Merged
Conversation
The inv() factory at tensor_functions.h:467 rejects rank != 2 and != 4
at construction with invalid_expression_error. The tensor_inv wrapper
constructor itself was silently accepting any rank — so direct
`make_expression<tensor_inv>(rank3_T)` constructions that bypass the
factory produced a "valid-looking" rank-3 node that misbehaved
downstream:
- The space-propagation block branches on r == 2 / r == 4 and silently
falls through for other ranks, leaving the result without an
annotation.
- Every consumer (evaluator, diff visitors, printer) had to grow its
own belt-and-braces rank check, or just produce wrong results.
Mirror the factory's rank gate in the wrapper ctor. Throws match the
factory's invalid_expression_error convention and — unlike assert —
fire in Release builds too.
Side effects:
- The belt-and-braces throw at tensor_differentiation_wrt_scalar.cpp:268
is now structurally unreachable. Kept as defense-in-depth with a
comment explaining the redundancy.
- TensorDiffWrtScalarTest.InvRank3ThrowsNotImplemented (from #291) is
no longer reachable as written — the make_expression call throws
before diff runs. Renamed to InvRank3RejectedAtConstruction and now
asserts the construction-time throw directly.
2 new wrapper-level lock-ins in TensorExpressionTest.h:
- InvWrapperCtorRank3Rejected
- InvWrapperCtorRank5Rejected
Surfaced by the code review of #291. Closes #292.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Review's MEDIUM: the belt-and-braces rank check at
tensor_differentiation_wrt_scalar.cpp is now structurally unreachable
after the wrapper ctor gate. Keeping it as "defense in depth" gives a
false sense of safety — there's no test that would fire if the
wrapper's gate were relaxed in the future, because the test path
catches the wrapper throw first.
Delete the dead branch. Replace with a one-line comment pinning the
rank ∈ {2, 4} contract on #292's wrapper ctor. Smaller surface, no
dead code, contract documented at the only enforcement point.
The parallel "rank-4 not implemented" throw in the *tensor-arg*
visitor (tensor_differentiation.cpp:347) is NOT dead — rank-4 inv-diff
w.r.t. a tensor remains deferred — and is left alone.
Full suite: 1334/1334 passing.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Address three findings from the pass-2 critical review:
- [HIGH] CHANGELOG entry contradicted the pass-1 commit. The entry said
the belt-and-braces throw was "kept as defense-in-depth" but pass-1
deleted it. Fix the wording to match reality and explain why the
surviving `if/else if/std::unreachable()` shape is the right
documentation of the rank ∈ {2,4} contract.
- [MEDIUM] The visitor's `else` branch silently applied the rank-4
contraction layout. If a future wrapper-gate relaxation admitted
rank-6, the `else` would emit a malformed AST instead of failing.
Replace `if (r == 2) ... else { rank-4 }` with `if (r == 2) ...
else if (r == 4) ... else std::unreachable()`. Same `std::unreachable`
pattern as projector_algebra.h:48. Caught in Debug as UB hint, abort
in sanitizer builds, vs silently-wrong in Release with the prior
shape.
- [MEDIUM] Wrapper-gate had rank-3 / rank-5 lock-ins but no low-end
coverage. tensor_expression accepts any rank std::size_t (no rank >= 2
invariant on the type itself), so rank-1 is technically constructible.
Add InvWrapperCtorRank1Rejected.
Full suite: 1337/1337 passing (+3 from rank-1 lock-in plus the
unreachable-branch addition).
The pass-2 NIT about the wrapper's space-propagation comment
("inv() factory rejects them") is left for a follow-up — fixing the
comment also wants the if/else if → if/else cleanup the original
review flagged as a NIT. Bundling those separately keeps each
commit's intent crisp.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
With the rank gate enforcing rank ∈ {2, 4}, the trailing "Other ranks:
inv() factory rejects them; this branch shouldn't be reached" comment
on the if/else if chain was stale — the wrapper rejects them, not the
factory, and the trailing fall-through is dead. Tighten to an
exhaustive if/else with a header comment crediting the gate.
Pure refactor — no behavior change. Full suite: 1337/1337 passing.
Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Pass-4 review LOW finding: rank-0 (tensor_expression accepts any size_t rank, including 0 — scalar-as-tensor degenerate form) was not in the wrapper-gate test matrix. Existing rank-1, rank-3, rank-5 lock-ins all exercise the same gate; rank-0 closes the low-end of the integer range. The PR body (also updated via gh api PATCH) now correctly reflects the pass-1 deletion of the visitor's belt-and-braces rank check; the prior body still claimed it was kept as defense-in-depth. Full suite still 1337+1 → 1338 tests passing (typed-test × 3 dims = 3 instances of the new rank-0 lock-in). Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
CI clang-format-18 flagged the <utility> include placement (line 13 between project includes) as the pass-3 reviewer's NIT predicted. Move it to the correct sort position. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
Pass-5 review flagged the doc-comment above the tensor_inv operator in tensor_differentiation_wrt_scalar.cpp as stale: it still described the belt-and-braces throw that pass-1 (commit d2ed390) deleted. After 4 prior review passes touching this file's code, the comment block was never refreshed. New text describes the actual contract: rank ∈ {2, 4} pinned by the wrapper ctor (#292); the body's std::unreachable() catches drift if a future wrapper relaxation admits a higher rank. PR body also updated via gh api PATCH to fix the test count (1337 → 1340, since the wrapper-ctor lock-ins are typed-tests × 3 dims) and remove the now-stale 'rank-0 not in matrix' line from the Follow-ups section. Signed-off-by: petlenz <peterlenz89.pl@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #292. Surfaced by code review of #291.
The
inv()factory intensor_functions.h:467already rejects rank ≠ 2 and ≠ 4 at construction withinvalid_expression_error. Thetensor_invwrapper constructor itself silently accepted any rank — so directmake_expression<tensor_inv>(rank3_T)constructions that bypass the factory produced a "valid-looking" node that misbehaved downstream (space-propagation block silently emitted nothing; every visitor needed its own belt-and-braces rank check).Now mirrored in the wrapper ctor with the same convention.
Cascade (after 5 review passes)
tensor_differentiation_wrt_scalar.cppunreachable, so it was deleted (not kept — pass-1 review flagged "dead code + zero coverage = worse than no defense"). The survivingif (r == 2) / else if (r == 4) / else std::unreachable()shape pins the rank ∈ {2, 4} invariant at the only enforcement point.if/else if(with a stale "shouldn't be reached" trailing comment) to exhaustiveif/else(pass-3 cleanup).TensorDiffWrtScalarTest.InvRank3ThrowsNotImplemented(from Rank-4 tensor_inv in scalar-arg diff visitor (#287) #291) renamed toInvRank3RejectedAtConstruction— themake_expression<tensor_inv>call now throws before diff runs.InvWrapperCtorRank0Rejectedto close the low-end test matrix.<utility>include placement) + stale doc-comment refresh.Test plan
TensorExpressionTest.InvWrapperCtor{Rank0, Rank1, Rank3, Rank5}Rejected— wrapper-level lock-ins on both sides of the {2, 4} band.TensorDiffWrtScalarTest.InvRank3RejectedAtConstruction— pins the construction-time throw.InvRank3SymbolStillThrowsuntouched (verifies the factory's independent gate).Follow-ups (not blocking)
m_rankontensor_expressionisprotected std::size_t(not const) — immutability is by convention, not language-enforced. A future hardening could mark itconstor add a// no setter existscomment.tensor_inv.h:17(and acrosstensor_if_then_else_*.h) wherethen_branch.get().dim()is read in the same mem-init list asstd::forward<Expr>(_expr). Re-analyzed as safe (thestd::forwardcast doesn't move; the actual move happens deep insideternary_op's base initialization), but worth a follow-up explanatory comment to head off future confusion.