refactor(language): group_sum becomes sum(group_by=) - #491
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (29)
📝 WalkthroughWalkthroughThis PR replaces ChangesGrouped sum migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 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 |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/design/ceiling.md`:
- Line 17: Remove the duplicate `sum` entry from the **Primitives** list,
keeping a single `sum` item that represents the operator with optional
`group_by` support.
In `@docs/models/monthly_budget.md`:
- Around line 207-209: Update the `sum` partition description in the monthly
budget documentation to qualify that only snapshots with non-null coordinates
belong to exactly one group; preserve the existing behavior that null-coordinate
terms are placed nowhere and contribute nothing.
In `@src/lpspec/relational/engines/polars/executor.py`:
- Line 531: Update the grouped-sum documentation to preserve the complete API
contract: in src/lpspec/relational/engines/polars/executor.py lines 531-531, use
sum(f, over=line, group_by=to) and sum(f, over=line, group_by=from); in
src/lpspec/relational/engines/polars/compiler.py lines 707-707, add over=line to
both network examples; and in src/lpspec/relational/engines/polars/compiler.py
lines 759-759, describe the operation as sum(..., over=..., group_by=...) or as
a grouped sum.
In `@tests/test_arithmetic_laws.py`:
- Around line 223-226: Update the explanatory comment near the mask test
description to remove the duplicated operator names and clearly distinguish the
two tested paths, using plain sum versus grouped sum with group_by or omitting
the parenthetical entirely.
In `@tests/test_group_sum.py`:
- Around line 101-104: Update the diagnostic regex assertions for the invalid
`group_by` cases in the test data to match `group_by=nope` and `group_by=to`
explicitly, rather than allowing the shorter `by=` suffix. Keep the existing
assertions for the `over` diagnostic and coordinate-scope behavior unchanged.
In `@tests/test_relational.py`:
- Line 652: Update the docstring of
test_two_sums_of_one_variable_collide_only_where_the_coordinates_meet to use the
current public syntax, replacing references to by=to and by=from with
group_by=to and group_by=from.
🪄 Autofix
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: a5aab3c6-62ec-4966-bcc5-31ff780a0d5f
⛔ Files ignored due to path filters (1)
examples/walkthrough.outis excluded by!**/*.out
📒 Files selected for processing (46)
bench/README.mdbench/cases.pybench/models/transport.yamldocs/ARCHITECTURE.mddocs/SPEC.mddocs/design/ceiling.mddocs/guide.mddocs/models/index.mddocs/models/monthly_budget.mddocs/models/pypsa_cyclic_storage.mddocs/models/pypsa_kvl.mddocs/models/pypsa_ramp.mddocs/models/pypsa_storage.mddocs/models/pypsa_transport.mddocs/models/transport.mddocs/models/tsp_mtz.mdexamples/README.mdexamples/monthly_budget.yamlexamples/ports/pypsa_cyclic_storage.yamlexamples/ports/pypsa_kvl.yamlexamples/ports/pypsa_ramp.yamlexamples/ports/pypsa_storage.yamlexamples/ports/pypsa_transport.yamlexamples/ports/tsp_mtz.yamlexamples/transport.yamlsrc/lpspec/language/dimensions.pysrc/lpspec/language/expression_parser.pysrc/lpspec/language/helpers.pysrc/lpspec/language/resolution.pysrc/lpspec/language/schema.pysrc/lpspec/linopy/builder.pysrc/lpspec/linopy/loader.pysrc/lpspec/lowering.pysrc/lpspec/relational/engines/polars/compiler.pysrc/lpspec/relational/engines/polars/data_validation.pysrc/lpspec/relational/engines/polars/executor.pysrc/lpspec/typeset/walk.pytests/golden/model.yamltests/test_arithmetic_laws.pytests/test_compiler.pytests/test_dimensions.pytests/test_group_sum.pytests/test_relational.pytests/test_typeset.pytests/test_validation.pytools/constructs.py
Merging this PR will improve performance by 29.78%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_emit[sector-s-lpspec-lp] |
8.7 MB | 6.7 MB | +29.78% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing refactor/sum-group-by (486f2ff) with main (0066264)
Footnotes
-
10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
One verb where there were two.
sumreduces a dim away, or — givengroup_by=— reduces it into the dim that coordinate targets.Why
group_by=and notby=Folding the grouping into
sumcosts something real: the verb no longersays a regrouping happened.
group_sumannounced it;sum(x, over=f, by=c)would hide it in a keyword easy to skim past, and the result's dims change
either way.
group_by=puts it back where it is visible.sum(p, over=generator, group_by=bus)reads as what it is, and the cost of the rename is a keyword fourcharacters longer than it had to be.
The retirement is the migration story
No alias, no deprecation cycle — per breaking changes are free. So the old
spelling has to fail at load naming its rewrite, which is checked
(
test_the_retired_group_sum_names_its_rewrite) in a way a shim would not be:A habitual
by=lands on the call-shape error, which quotes the usage lineincluding
group_by.Shape of the change
The surface collapses; the plan does not.
SumandGroupSumstay twonodes, because reducing a dim away and reducing it into another are different
relational shapes and the executor cases were never what the surface was
conflating. What changed is one
Builtingaining an optional coordinatekwarg, and the dim algebra and lowering branching on its presence.
One small generalisation fell out:
Builtingainedoptional_coordinate_kwargs, since every existing coordinate kwarg wasmandatory.
A note on how this was done
The mechanical part was a corpus-wide rename over 42 files, and it collided
four times. Each was caught by a gate, and each is worth naming because a
blanket replace on an identifier that is a substring of its replacement is a
trap:
_helper_group_sum→_helper_sumsumcalls the grouping helpertest_sum_over_typo_is_rejectedruffF811RETIREDkey'group_sum'→'sum'sumclaims to be retiredtools/constructs.pycolumnThe first and third are the ones that would have shipped quietly.
Scope and sequencing
This is a breaking surface change, and #432 exists to sequence those as one
window rather than piecemeal. Taken now deliberately; #332 and #432 get a note
so the remaining renames are decided against what actually landed rather than
against the old shape.
Branched from
main, so it does not includeat()(#489). The two touch thesame five files and whichever lands second rebases —
at's own keyword question(
onto=vsover=) was settled in that PR for the same reason this one settlesgroup_by=: one keyword should not mean two directions.806 passed,
ruff/pyrefly/mkdocs --strictclean, goldens and generatedtables regenerated.
Summary by CodeRabbit
sum(..., group_by=...).