feat(language): one shift(over=, by=, edge=), replacing roll and shift - #359
Merged
Conversation
**Breaking: `roll()` is gone and `shift()` changes shape.** No `!` marker — the alpha stream refuses one (RELEASING.md), and the break is described here instead. roll and shift were always one IR node — plan.Translate, named for the coordinate map rather than either spelling. The surface kept two names, and the corpus picked the wrong one six times across four ports, needing a four-line comment in pypsa_ramp to justify the choice. Two names for one node cause real mistakes in the most carefully-reviewed files here. shift(soc, over=snapshot, by=1) vacated positions absent shift(soc, over=snapshot, by=1, edge=wrap) cyclic shift(seg, over=bp, by=1, edge=0) vacated positions contribute 0 `edge` carries all three policies in one keyword instead of `wrap` and `fill` as two. That is the part worth the diff: the pair could contradict each other — cyclic *and* fill what it vacates — and the old surface had to refuse that combination at run time. Now it has no spelling. One keyword, three legal values, nothing to check. The dimension also moves out of the kwarg *key*. `roll(x, snapshot=1)` named its dim in the key, which is why a macro could not parameterise it and why ARCHITECTURE cited it as the language's own counterexample on macro-friendliness. `over=` is an ordinary value, so a formal can carry it, and helpers.py loses dimension_is_key and split_dimension_key with it. EdgeNode joins the resolved AST beside DimensionNode and CoordinateNode: a closed keyword that names neither data nor a coordinate. Adding it to the union made four exhaustive walkers incomplete, which is the type system doing its job — expansion, validation, the eager evaluator and the typesetter each say what an edge policy is not. The typeset goldens are byte-identical: the same math renders the same way, which is the check that this is a surface change and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 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 ignored due to path filters (1)
📒 Files selected for processing (38)
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 |
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
…heir line (#361) What survived rebasing onto #359 — the engine-side half of this pass (the label arithmetic, `plan.children`, `dataclasses.replace` over `TermFragment`, the shared shift-edge filter) landed with that branch, and the duplicated `_check_where` call it also fixed is upstream too. DRY: - The "Did you mean …?" clause was written three times — for an unknown declaration name, an unknown YAML key, and an unknown symbol-table entry. `errors.did_you_mean` is now the clause and each caller keeps its own framing sentence, which is the part that genuinely differs. `errors.py`'s own docstring had already noted there were two copies before it added a third. Drops the `difflib` import from `schema.py` and `typeset/symbols.py`. - `_fold_and` and `_fold_or` differed only in the node class; `_folder` takes it as an argument. The fold is the grammar's associativity, which is one rule. - `_resolve_where` tested `isinstance(value, str) and ns.kind(value) == ...` three times in sequence for the three things a where-comparison's RHS may not name. One walrus and a `_declared_rhs_error` dispatch; the three messages stay distinct, because only the dimension case is a silent wrong answer rather than an obvious one. - `Namespace._unknown` built its two kind labels and their two value lists through parallel ternaries that had to stay in step; one list of pairs. Comments: removed the ones that restate the line below them — the per-`isinstance` branch labels in `_coerce_to_dataarray`, `# Step 2:` and `# Step 5:` numbering a sequence that no longer exists, `# Atoms` / `# Unary` / `# Function calls` over the grammar they name, `# Referenced dimensions must be declared`. Every comment carrying an argument stays, and where a section label was doing real work it was rewritten into one instead (`# at most one comparison, and only at the top`, `# NOT binds tightest, then AND, then OR`). Also drops `expression_parser.RESOLVED_REFERENCE_NODES`, unused, and whose comment claimed both consumers asserted on it when neither did. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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.
Implements rename 1 from #332.
The surface
Why one keyword rather than
wrap=+fill=#332 proposed
shift(…, by=1, wrap=true)alongside the existingfill=. That re-creates a contradiction the two names used to prevent: "cyclic, and fill what it vacates" was nearly unwritable asroll(x, d=1, fill=0), and got a refusal that could say why. As two keywords on one call it reads plausible and needs a run-time check.edge=collapses them: one keyword, three legal values, no illegal combination to refuse. A cyclic map vacates nothing, and the surface now says so by construction.Why unify at all
rollandshiftwere already one IR node —plan.Translate, whose docstring says it is "named for the coordinate map rather than for either spelling". Only the surface was doubled, and the corpus picked the wrong member six times across four ports, needing four lines of comment inpypsa_ramp.yamlto justify the choice. That is two names for one node causing real mistakes in the most carefully-reviewed files in the repo.It also moves the dimension out of the kwarg key.
roll(x, snapshot=1)named its dim in the key, which is why a macro could not parameterise it —ARCHITECTURE.mdcited exactly this as the language's own counterexample on macro-friendliness.over=is an ordinary value, so a formal can carry it, andhelpers.pylosesdimension_is_keyandsplit_dimension_key.What checks this is a surface change and nothing else
The typeset goldens are byte-identical. Only the source line in
tests/golden/model.yamlchanged; all three rendered outputs are unchanged, because the math is unchanged. All 11 ports still match their published optima.EdgeNodeA closed keyword joins the resolved AST beside
DimensionNodeandCoordinateNode— it names neither data nor a coordinate. Adding it to theArithmeticNodeunion made four exhaustive walkers incomplete, which is the type system doing its job: expansion, validation, the eager evaluator and the typesetter each now say what an edge policy is not.Migration
roll(x, d=n)→shift(x, over=d, by=n, edge=wrap)·shift(x, d=n)→shift(x, over=d, by=n)·shift(x, d=n, fill=v)→shift(x, over=d, by=n, edge=v)699 tests pass; ruff and pyrefly are at parity with
main.🤖 Generated with Claude Code