Repository navigation
perf(sinks): a row is seated the way a column is, for both solvers - #421
Merged
Merged
Conversation
FBumann
added
area:engine
|
Warning Review limit reached
Next review available in: 19 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 (3)
📝 WalkthroughWalkthroughThe change adds dense row projections and reusable row blocks to ChangesDense row solver integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ModelTables
participant GurobiSink
participant HiGHSSink
participant SolverAPI
ModelTables->>GurobiSink: Provide dense_rows and row_blocks
GurobiSink->>SolverAPI: Add CSR constraint blocks
ModelTables->>HiGHSSink: Provide dense_rows and row_blocks
HiGHSSink->>SolverAPI: Add row bounds and matrix slices
Possibly related PRs
🚥 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 |
FBumann
marked this pull request as draft
July 31, 2026 16:32
Collaborator
Author
|
Parked as draft — #418 moves Worth noting for whoever picks it up: the Gurobi sink has the same pattern, # sinks/solvers/gurobi.py:162
ordered_rows = model.rows.sort('row')
ordered_matrix = model.matrix.sort('row')
for lo, hi in model.row_chunks_by_nonzeros(batch):
rows = ordered_rows.filter(pl.col('row').is_between(lo, hi, closed='left')).select(...)
a = ordered_matrix.filter(pl.col('row').is_between(lo, hi, closed='left'))Both solvers sort the row frame and then scan it once per chunk to find a range
|
`dense_columns` exists because `col` is dense `0..n-1` and so *is* the position
a value belongs at. `row` is dense in exactly the same way, and neither solver
was treating it that way: both sorted the row frame and then filtered it once
per chunk, so `fleet/l` read the same 6M rows nine times over to hand each of
them over once.
So the row half joins the column half in the contract, where tables.py already
argues it belongs: two sinks computing the same projection separately could
disagree about the model they loaded.
ModelTables.dense_rows(infinity) -> (sense, rhs) over the solver's row index
ModelTables.row_blocks(budget) -> each chunk of rows with the entries it owns
`dense_rows` stops at the sense rather than at bounds because that is where the
two solvers part — HiGHS wants a row's lower/upper, Gurobi its comparison and
right-hand side, and both are that pair spelled differently. `SENSE_CODES` is
the spelling neither owns; it was already inside gurobi.py as a per-chunk
`replace_strict`.
`row_blocks` orders the matrix once and locates each chunk by binary search on
the label column. Searched in polars rather than through numpy: pulling the
column out to search it there is marginally faster and holds a second copy of
it for the whole loop, 0.11 GB at `transport/l`, which is the wrong trade in a
pass that exists to stay bounded.
Paired in-process, alternating, min of five, same frames:
highs fleet/l 6.0M rows / 14.0M nz 0.722s -> 0.644s -10.8%
transport/l 1.4M rows / 12.6M nz 0.354s -> 0.308s -13.0%
nodal/l 1.0M rows / 3.0M nz 0.140s -> 0.131s -6.7%
sector/l 1.0M rows / 1.0M nz 0.058s -> 0.054s -6.9%
dispatch/l 0.1M rows / 10.0M nz 0.403s -> 0.379s -6.0%
fleet/m 0.063s -> 0.059s -6.9%
**Gurobi does not move**, and that is worth stating rather than hiding: its own
`addMVar`/`addMConstr` take 1.7 s where HiGHS takes 0.06 s for the same model,
so the polars work this removes is invisible inside it. The sharing is
justified by the duplication, not by a second speedup.
Peak is unchanged on both — the dense row vectors replace a sorted copy of the
row frame, and what is left is inside the run-to-run spread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
force-pushed
the
perf/polars-highs-rows-by-position
branch
from
July 31, 2026 17:17
7566848 to
1bd07f2
Compare
FBumann
changed the title
FBumann
marked this pull request as ready for review
July 31, 2026 17:17
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lpspec/relational/sinks/solvers/gurobi.py (1)
164-173: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDerive Gurobi operator positions from
SENSE_CODES.Line 166 duplicates the numeric order from
src/lpspec/relational/sinks/tables.py. IfSENSE_CODESchanges, Gurobi can apply an incorrect operator without an error. ImportSENSE_CODESand build the operator lookup from its values.🤖 Prompt for 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. In `@src/lpspec/relational/sinks/solvers/gurobi.py` around lines 164 - 173, Update the Gurobi solver’s operator lookup near the `spelling` definition by importing and using `SENSE_CODES` from the relational sinks tables module, deriving the array positions from its values instead of hardcoding the LESS_EQUAL, GREATER_EQUAL, EQUAL order. Keep the existing `sense[...]` indexing and `addMConstr` flow unchanged.
🤖 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.
Nitpick comments:
In `@src/lpspec/relational/sinks/solvers/gurobi.py`:
- Around line 164-173: Update the Gurobi solver’s operator lookup near the
`spelling` definition by importing and using `SENSE_CODES` from the relational
sinks tables module, deriving the array positions from its values instead of
hardcoding the LESS_EQUAL, GREATER_EQUAL, EQUAL order. Keep the existing
`sense[...]` indexing and `addMConstr` flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e01bb86f-c32b-4f1e-82f0-fed33427b59d
📒 Files selected for processing (4)
src/lpspec/relational/sinks/solvers/gurobi.pysrc/lpspec/relational/sinks/solvers/highs.pysrc/lpspec/relational/sinks/tables.pytests/test_relational.py
The array `addMConstr` indexes was written out in `SENSE_CODES`' order, so the two had to be kept in step by hand. Nothing checks that: a wrong order is a model whose comparisons are silently permuted, and every solver would answer it confidently. Built from the mapping instead, keyed by name. A sense added to `SENSE_CODES` and not to `_GUROBI_SENSE` raises where it is built rather than mislabelling a row. Raised by CodeRabbit on #421. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Good catch — fixed in 0a30684. It is the failure mode this repo argues against The table is now built from the mapping rather than retyped in its order: _GUROBI_SENSE = {'<=': 'LESS_EQUAL', '>=': 'GREATER_EQUAL', '==': 'EQUAL'}
def _spelled(gurobipy):
spelling = np.empty(len(SENSE_CODES), dtype='<U1')
for sense, code in SENSE_CODES.items():
spelling[code] = getattr(gurobipy.GRB, _GUROBI_SENSE[sense])
return spellingOrder-independent, and a sense added to |
#434 landed while this branch was open, and the two changes disagree about the same function for good reasons on both sides. #434 measured that Gurobi's `addMConstr` charges ~42 ns per *model column* per call whatever the block holds — 0.23 s in one call against 0.89 s in forty — and stopped batching it. This branch moves both solvers onto a shared `row_blocks` so they cannot come to disagree about the model they load. Resolved by putting #434's finding *into* the shared contract rather than around it: `row_blocks(budget=None)` yields one block, and its docstring says that whether splitting pays is a property of the API being fed and not of the model — HiGHS's budget bounds a temporary, Gurobi's per-call cost scales with the model's width. Both answers now come out of the same code, and `gurobi.py` passes `batch_rows` straight through un-defaulted. row_blocks(None) -> 1 block addMConstr calls: 1 row_blocks(5) -> 4 blocks addMConstr calls: 4 batch_rows=1 addMConstr calls: 20 objective 190.0 in every case Kept from this branch over main's: the Gurobi sense spelling derived from `SENSE_CODES` rather than written out beside it. main's version is a literal array in an order that has to match by hand, and a wrong order there is a model whose comparisons are silently permuted — which every solver answers confidently. 752 passed, 23 of them Gurobi's, ruff + pyrefly clean.
FBumann
enabled auto-merge (squash)
July 31, 2026 17:59
FBumann
added a commit
that referenced
this pull request
Jul 31, 2026
…ug made (#441) * docs(bench): re-measure the published tables, and reverse a conclusion The committed results were taken at `ef8558c` — version `0.0.0a27`, before the package was even renamed. Eight perf changes have landed since (#408, #412, #413, #414, #415, #434, #421, #433), so every table on the page understated the engine, and one paragraph drew a conclusion the numbers no longer support. Re-run in one sitting on an idle machine: the ladder, the density sweep and the scaling run, so all three files describe the same tree. Parity gate passes on all six cases (`fleet` to 4.6e-16) before anything is timed. **What moved**, against the previously published numbers, `l` rung, our arm: wall -10% to -39% on nine of twelve rungs, peak -2% to -21% on nine. **What got worse, and is not buried.** `dispatch` build is 10-12% slower — that is `maintain_order='left'` from #433, which buys -21% peak on the same rung and is the trade that PR argued for. `fleet/highs` peak is up 8.2%, outside its 1.0% floor, and I have no explanation for it. **A conclusion is reversed.** `## Absurd sizes` read 1.07x peak at 120M variables and concluded that our memory headroom "is a small-and-sparse-model property, not a scaling one". Peak is now 0.73-0.83x across four orders of magnitude. Two things moved it and only one is us: #433 took ~20% off this case, and the earlier reading came from a run *that section itself flagged as noisy* — it recorded linopy at 38.5 s and 106.2 s for the same rung. The old number was a conclusion drawn across the noise floor. The section now says so rather than quietly presenting a better one. **`profiled` is no longer the case in the ladder we lose** (1.09x -> 0.74x through the highs sink), and `fleet/lp` has crossed on wall (1.03x -> 0.90x). Both paragraphs are rewritten to say what changed rather than to delete the sentences that admitted the losses. Two numbers still go against us and are still named: `profiled` 1.31x on wall through the LP sink, `fleet` 1.47x on peak. Not touched: `## Sink capabilities` still says Gurobi's row wants a spike before it is relied on. #418 shipped a real Gurobi sink since, so that caveat is probably obsolete — but exercising it is not this change, and asserting capabilities I have not run is how that table would come to lie. * docs(bench): re-measure at 98f382d, and correct a claim the harness bug made Every number on the page now comes from one run at `98f382d` on a clean tree, and the page says so — the fingerprint hash is what makes a published number traceable to the code that produced it, and the previous one recorded `782f76b-dirty`, which identifies nothing. **The `highs` comparison was not like for like, and it flattered us.** #438 found that linopy's `to_highspy()` was being timed *with* variable naming, which neither of our solver sinks does — 82% of its hand-off, and memory as well as time. Corrected: highs sink, l rung was now dispatch peak 0.87x 1.46x fleet peak 0.76x 1.10x profiled peak 0.82x 1.27x transport peak 0.75x 1.10x So the summary changes from "ahead on both, on all six" to **ahead on wall on five of six, and behind on peak on four of six through `highs`**. HiGHS's own dense model dominates both arms there and our COO triples are what is left. The `lp` sink barely moved, because that comparison was already like for like, and it is where the representation actually pays: ahead on peak on four of six. `profiled` is the case in the ladder we lose, on both sinks (1.27x, 1.07x). Said plainly rather than left to the tables. The scaling section's reversal stands and is re-measured: peak 0.68-0.78x from 10k to 120M variables, against the 1.07x this page used to conclude the advantage "runs out" from. That section now also says it is the LP sink, and therefore not evidence for the `highs` peak row.
FBumann
added a commit
that referenced
this pull request
Sep 10, 2026
… the head of energy-models/mathspec#421 (#1565) > **Prompt:** Lets check if lpspec needs to adjust anything after we merge the stack below energy-models/mathspec#421 — Do a Draft pr pinning the head of #421 > [!NOTE] > The following content was generated by AI. The pin is now the release tag `v0.0.0-alpha.82`, which carries the whole five-PR stack for math-spec's issue #401 (#415, #416, #418, #420, #421 — the last squash commit, a2dcee7, is an ancestor of the tag). It replaces the commit `37f8b5136f8433a56653c1954d7ffe770902dd40`, the head of [energy-models/mathspec#421](energy-models/mathspec#421), that this PR pinned while no release contained the stack. lpspec needs no code change. Nothing it imports from math-spec moved; the one consumer-visible change is that math-spec's Markdown typesetter now escapes a `description:` the way its LaTeX and Typst formats already did (`_`, `[`, `]`, `*` set as characters, a name in backticks as a code span). So this PR is: - the pin, in `pyproject.toml` and the three math-spec lines of `uv.lock` - the regenerated pages (`tools.gallery_math`, `tools.ladder`): every changed line is an escape such as `'bp\_x'` or `\[0, 1\]` - two descriptions written plain, in `examples/ports/pypsa_cvar.yaml` and `examples/ports/pypsa_losses.yaml`, because their `*and*` now sets as literal asterisks (and never set as emphasis in LaTeX or Typst); the same two lines inside the YAML block the gallery pages embed verbatim, which no tool writes Against the commit pin the full suite on `main` at 2928e08 was 3754 passed, 251 skipped, 1 xfailed, 2 failed, the two failures being the stale generated pages this PR regenerates. <details><summary>Verified at the tag</summary> - `uv lock` resolves math-spec to `v0.0.0a82` (commit 140ec94e); locked with uv 0.12.11 so the lock diff is the three math-spec lines only (older uv versions rewrite marker annotations across the file) - `uv sync --all-extras` - `uv run python -m tools.gallery_math`, `uv run python -m tools.ladder`, `uv run python -m tools.constructs`: no page changed against the commit pin, so the tag's tree is the pinned commit's tree for everything lpspec renders - `uv run pytest -q -n auto tests/test_models_gallery.py tests/test_pypsa_ladder_page.py tests/test_typeset_gallery.py tests/test_docs_site.py tests/test_ports.py`: 612 passed, 14 skipped (the typst binary, and the MILP ports that record no duals) - `ruff check .`, `ruff format --check .`, `pyrefly check` on both configs: clean - Left alone on purpose: the `*and*` in the comment of `examples/symbols/transport.yaml` and the bold sentence in the hand-written prose of `docs/examples/pypsa_losses.md`, neither being a description - Not run here: the full suite under pixi, and the typst-backed typeset tests </details> <details><summary>The next math-spec release needs a follow-up in lpspec</summary> math-spec's `main` already carries #427 (`refactor(program): a variable's domain is called domain in the program`), which renames `VariableDeclaration.variable_type` to `domain`. It is **not** in alpha.82, so this pin is unaffected. lpspec reads `variable_type` in `src/lpspec/linopy/builder.py` at three sites (two in the variable attach, one in the discrete-variable listing before the solve), and the next math-spec release will need a follow-up in lpspec that renames those reads. Not done here. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019d4fAY3RikKY317uerZ2Hc https://claude.ai/code/session_017tCiQ1MTt4jG8X3qNEGNvZ --------- Co-authored-by: Claude <noreply@anthropic.com>
fluxopt-release-bot Bot
added a commit
that referenced
this pull request
Sep 10, 2026
🤖 I have created a release *beep* *boop* --- ## [0.0.1-alpha.317](v0.0.1-alpha.316...v0.0.1-alpha.317) (2026-09-10) ### Documentation * descriptions set as prose under math-spec's escaping, pinned to the head of energy-models/math-spec[#421](#421) ([#1565](#1565)) ([5c6b606](5c6b606)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: fluxopt-release-bot[bot] <307443024+fluxopt-release-bot[bot]@users.noreply.github.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
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.
Rebuilt on top of #418 — the original version of this PR predated the
solvers//writers/split and targetedsinks/highs.py. Rewriting it againstthe new structure turned out better than porting it would have: the change
belongs in the shared contract, not in one solver.
What
dense_columnsexists becausecolis dense0..n-1and so is the positiona value belongs at.
rowis dense in exactly the same way — and neither solverwas treating it that way:
fleet/lread the same 6M rows nine times over to hand each of them over once.So the row half joins the column half in
tables.py, where its docstringalready argues it belongs — two sinks computing the same projection separately
could disagree about the model they loaded:
ModelTables.dense_rows(infinity)(sense, rhs)over the solver's row indexModelTables.row_blocks(budget)dense_rowsstops at the sense rather than at bounds because that is wherethe two solvers part: HiGHS wants a row's lower/upper, Gurobi its comparison and
right-hand side, and both are that pair spelled differently.
SENSE_CODESisthe spelling neither owns — it was already in
gurobi.py, as areplace_strictrun once per chunk.
row_blocksorders the matrix once and locates each chunk by binary search onthe label column. Searched in polars, not through numpy: pulling the column
out to search it there is marginally faster and holds a second copy of it for
the whole loop — 0.11 GB at
transport/l, the wrong trade in a pass that existsto stay bounded. I measured that version first.
Numbers
Paired in-process, alternating, min of five, same frames:
The gain tracks rows-per-nonzero, which is the shape of what was removed:
dispatchhas 100 nonzeros per row and gains least.Gurobi does not move. Worth stating rather than burying: its own
addMVar/addMConstrtake 1.7 s where HiGHS takes 0.06 s for the samefleet/mmodel, so the polars work removed here is invisible inside it (all five cases
land within ±1.3%, which is noise at that ratio). The sharing is justified by
the duplication, not by a second speedup — but Gurobi is strictly doing less
work than before, and if gurobipy ever stops dominating, it inherits the win.
Peak is unchanged on both. The dense row vectors replace a sorted copy of
the row frame; what's left is inside the run-to-run spread (
transport/lminsover four samples: main 2.95–3.10 GB, this PR 3.00–3.09 GB).
Test
test_a_row_with_no_terms_keeps_its_seat_at_any_chunking, parametrised overSOLVERS× four batch sizes. A masked variable orphans a constraint row, thislane keeps it, and
0 == 5must stay infeasible. Rows are now handed over byposition, so a row that fell out of the frame would take a comparison nothing
can fail, leave the constraint unenforced, and the model would come back solved
against one that cannot be. Ragged batches because the range loop is where a
seat would be lost; a round number is the one split that hides an off-by-one.
751 tests pass, including the 23 Gurobi ones.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests