test(laws): the absence rules under wider shapes, starting with group_sum - #316
Conversation
…_sum Follow-up to #314, which routed `group_sum` through the same absence propagation as `sum` on the argument that a group *is* a sum — and shipped it with no test. Its behaviour was a claim rather than a result; it is now a result: the two spellings separate 640 against 480, and both lanes agree about where each lands. The cases in #314 all reduced over `f` with one mask on `f`, which is the smallest arrangement that shows the rule and small enough to pass for the wrong reason. Five more, each varying one thing: - **group_sum** rather than sum — the untested path above - **two masks that do not nest**, so the summand needs the intersection rather than any one of them. The implementation collects one restriction per fragment and applies every one to every fragment; a version that stopped at the first, or composed them pairwise down the addition tree, passes every single-mask case - **a broadcast coefficient**, which must not move presence at all — a sparse parameter is encoding, not absence (SPEC §6), so the separation has to come from the masked variable alone - **a mask on a dim the reduction does not touch** — mask on `t`, reduce over `f`. The restriction is keyed by the dims the presence names, so this works; keying it by the reduced dim would silently do nothing here - **absence created by `shift`** rather than by a mask, because two sources of "not here" that behaved differently inside a reduction would mean SPEC §6 describes only one of them All five agree across both lanes and the LP re-solve, so this adds coverage rather than fixing anything — which is the outcome worth recording, since the group_sum path had no evidence either way. One fixture note: `g` and the coordinate reaching it exist only for the grouped cases. A dimension no declaration uses has no coordinate set to check against and the executor refuses it, correctly.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds wide-shape arithmetic-law regression coverage for absence propagation across grouped reductions, intersecting masks, broadcasting, non-reduced dimensions, and shifted variables. ChangesArithmetic absence semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_arithmetic_laws.py`:
- Around line 340-362: Update
test_shift_created_absence_reaches_a_reduction_like_any_other so the
constraint’s reduced expression uses a separate variable only through shift(x,
t=1), while the objective continues maximizing x. Adjust the expected objective
to 320.0, preserving the test’s isolation of the absent shifted summand and
distinguishing correct reduction propagation from a missing restriction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cc38228c-f17b-4261-979e-2f7fc2aca6ab
📒 Files selected for processing (1)
tests/test_arithmetic_laws.py
As written it could not fail. `sum(x + shift(x, t=1), over=f) <= 120` bounds x[.,0] through the t=1 row whether or not absence propagates at t=0, so the t=1 row dominates and the objective reads 120 either way. Measured rather than argued: neutering `_propagate_absence` fails six of the sixteen cases in this file, and this was not one of them — a test guarding a rule it cannot observe. The shifted operand is now a separate variable from the one the objective maximises, so t=0 is the only place the rule can show. Propagated, the summand is absent there and x[.,0] is free to its bounds; without it the row survives as `sum(x, over=f) <= 120` and caps them, giving 240 against 320. Under the same neutering it now fails, and fails on the differential assertion — the two lanes disagree — rather than on the numeric one, which is the stronger signal. Seven of sixteen now catch the regression.
|
Good catch — the finding is valid, and worse than "adjust the expected value": the test could not fail. Verified rather than assumed
Measured by neutering A test guarding a rule it cannot observe. It would have gone on passing through any regression of the thing it names. The fixThe shifted operand is now a separate variable from the one the objective maximises, so t=0 is the only place the rule can show: sum(x + shift(v, t=1), over=f) <= 120 # objective still maximises x
Expected value updated to 320.0 as suggested. One note on the suggested shape: keeping Re-verified under the same neuteringand it fails on the differential assertion (the two lanes disagree), not the numeric one — a stronger signal than the value being off. Gates: 653 passed · ruff clean. Only |
1 participant
Follow-up to #314.
The one that had to exist
#314 routed
group_sumthrough the same absence propagation assum, on the argument that a group is a sum — and shipped it with no test. That made its behaviour a claim rather than a result.It is now a result, and it holds: the two spellings separate 640 against 480, and both lanes agree about where each lands.
The rest
Every case in #314 reduced over
fwith a single mask onf. That is the smallest arrangement that shows the rule, and small enough to pass for the wrong reason. These vary one thing each:group_suminstead ofsumw * x + y)yalonet, reduce overf. The restriction is keyed by the dims the presence names, which is what makes this work; keying it by the reduced dim would silently do nothing hereshiftResult
All five agree across both lanes and the LP re-solve. This adds coverage rather than fixing anything — which is the outcome worth recording, because the
group_sumpath had no evidence in either direction before it.One fixture note worth keeping:
gand the coordinate that reaches it exist only for the grouped cases. A dimension no declaration uses has no coordinate set to check against, and the executor refuses it — correctly, so the fixture bends rather than the engine.Gates: 653 passed, 4 skipped, 1 xfailed · ruff clean · pyrefly 0 errors.
Summary by CodeRabbit