Repository navigation
perf(polars): two group_sums collide only where the coordinates meet, and the matrix leaves in row order - #487
Conversation
…oordinates meet #408 established that two fragments can share a solver column only if they carry the same variable. Sharing a variable is necessary and not sufficient, and the assembly treated it as both — so `transport`'s balance, `group_sum(f, by=to) - group_sum(f, by=from)`, sorted every nonzero in the model to discover that nothing repeated. At `l` that is 0.20 s of a 0.69 s constraint, and the answer is always no. Two fragments of one variable reach the same row only if the operators that moved their labels send them there. A fragment now records those operators, so mappings that differ **only in a coordinate** ask the question they actually pose: is there a line whose two ends are one bus? That is a scan of the `line` table — forty rows at `l`, against 12.6M nonzeros. Everything else answers yes. A shift on one side and not the other, a reduction that left them over different dims, a product that broadcast one wider — each moves a label in a way this does not model, and the cost of being wrong is a matrix holding one cell twice, which an LP reader sums and a solver handed duplicates may do either way. `may_share_a_column` also answers *no* for distinct variables, which is #408's half of the same question, so the assembly asks one thing rather than two. **Not a win on its own** — it is measured with the row-ordering that follows, because dropping the sort alone hands the sink an unordered matrix and it sorts instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 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 selected for processing (4)
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 |
Merging this PR will improve performance by 13.01%
Performance Changes
Tip Curious why this is faster? Comment Comparing 
Footnotes
|
Every sink reads the matrix a row range at a time, so one handed an unordered matrix orders it — a second pass over a finished frame, and a second copy of it while the solver's own model is resident. Ordering it in the build happens inside the pipeline that is already materialising the rows. A lone term gets the order from its *join*: the row frame is in label order since #433, and `maintain_order='left'` keeps it — half the cost of sorting the result on `dispatch/l` (0.068 s against 0.129 s). Several terms are several ordered runs stacked, and runs are not an order, so those sort. Only the lone term asks the join for anything, because a stack pays the sort regardless and the join's guarantee is then dead weight. `_in_row_order` states the claim after the stack, the way `_in_label_order` does for the labels: the concatenation is ordered by construction — ascending row ranges of ordered shares — but polars cannot see that through a `concat`, so it is verified with a linear scan and then flagged. The sort behind it is the correctness floor and is expected never to run. This is what makes the coordinate test in the previous commit pay. Measured together on `transport/l`, in-process and paired: today: sort(row,col) + check build 0.975s handoff 0.488s total 1.463s coordinate test alone build 0.466s handoff 0.956s total 1.422s both build 0.686s handoff 0.531s total 1.217s Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4f6b552 to
cd65349
Compare
Two changes that only pay together, and a claim about ordering that is checked rather than assumed.
1. Sharing a variable is necessary for sharing a column, not sufficient
#408 established that two fragments can share a solver column only if they carry the same variable. The assembly treated that as sufficient too — so
transport's balance,group_sum(f, by=to) - group_sum(f, by=from), sorted every nonzero in the model to discover that nothing repeated, and the answer is always no.Two fragments of one variable reach the same row only if the operators that moved their labels send them there. A fragment now records those operators (
('group', over, coordinate, into),('shift', dimension, by, wrap)), so mappings differing only in a coordinate ask the question they actually pose: is there a line whose two ends are one bus? That is a scan of thelinetable — forty rows atl, against 12.6M nonzeros.Everything else answers yes. A shift on one side and not the other, a reduction that left them over different dims, a product that broadcast one wider — each moves a label in a way the mapping does not model, and the cost of being wrong is a matrix holding one cell twice. The default is the sort.
may_share_a_columnalso answers no for distinct variables, which is #408's half of the same question, so the assembly asks one thing rather than two.2. The matrix leaves the build in row order
Every sink reads the matrix a row range at a time, so a sink handed an unordered matrix orders it — a second pass over a finished frame, and a second copy of it while the solver's own model is resident. Ordering it in the build happens inside the pipeline already materialising the rows.
A lone term gets the order from its join: the row frame is in label order since #433, and
maintain_order='left'keeps it. Several terms are several ordered runs stacked, and runs are not an order, so those sort. Only the lone term asks the join for anything, because a stack pays the sort regardless and the join's guarantee is then dead weight._in_row_orderstates the claim after the stack, the way_in_label_orderdoes for the labels: the concatenation is ordered by construction — ascending row ranges of ordered shares — but polars cannot see that through aconcat, so it is verified with a linear scan and then flagged. The sort behind it is the correctness floor and is expected never to run.The first change is not a win on its own: dropping the sort hands the sink an unordered matrix, and the sink sorts instead. That is why they are one PR.
What is actually measured
Two paired runs of
transport/l,--arms lpspec, on this tree (post-rebase), taking min of five rounds. The honest comparison is each side's run-to-run spread on identical code against the gap between sides:test_rebuildtest_emit[…-highs]test_emit[…-lp]The build speedup is real and reproducible: ~10–15%, comfortably clear of noise, and it is the one number to quote.
Both emit paths are inconclusive at n=2 — opposite orderings across the two pairs, and
-lpon the branch swung 17% between two runs of the same binary. The noise exceeds the effect. More rounds would settle it; nothing here should be published as an emit result.Reconciling with the commit messages
The commits carry a larger figure — build
0.975 s → 0.686 s(−29.6%), total1.463 s → 1.217 s(−16.8%). Those were taken on the pre-rebase tree, before #480, with a build/handoff split that is not the harness'srebuild/emitsplit. They are stale rather than wrong, but the measured win on the tree being merged is about half of the build claim. If you want the commit bodies amended to the reproduced number before merge, say so — it needs a force-push.Memory, from CodSpeed
CodSpeed runs here in
mode: memory(.github/workflows/codspeed.yml), so it does not measure the speedup above. What it reports is a net −0.28%, and a split that is not noise-shaped:test_emit[profiled-s-lpspec-**lp**]test_emit[transport-s-lpspec-**lp**]test_emit[storage-s-lpspec-**highs**]test_emit[fleet-s-lpspec-**highs**]Both regressions are the LP-file sink and both improvements are the direct-solver sink, which follows from the rationale rather than contradicting it: the saving is a second copy avoided while the solver's own model is resident. With HiGHS it is; writing an LP file there is no solver model to be concurrent with, so the ordering is simply paid earlier, inside the build. That trade is a consequence of the design and is stated here rather than left for a reviewer to find.
CodSpeed also flagged "different runtime environments detected" over exactly the benchmarks that moved, so those four deserve a re-run before being taken as settled.
Correctness
The risky claim is the negative one — skipping the aggregate when
may_share_a_columnsays no. Both directions are pinned: a balance over a network with no line to itself takes the fast path, and the same balance with one collides and aggregates. Plus a compiler-level test that two fragments of one variable are compared by what moved their labels rather than by the variable alone.Rebase note
Written against
53f8eb8, rebased here onto currentmain, 22 commits later. That interval includes #480 (a masked-out scalar variable takes its row with it), which touched both files this changes plustests/test_relational.py. The rebase applied clean and the suite is green, but no textual conflict is not no semantic interaction when both edit fragment construction — that overlap is the thing to look at first.788 passed ·
ruffclean ·pyrefly0 errors.🤖 Generated with Claude Code