Repository navigation
docs(bench): re-measure at 98f382d, and correct a claim the harness bug made - #441
Merged
Merged
Conversation
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.
FBumann
added
docs
|
Warning Review limit reached
Next review available in: 28 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 (5)
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 |
#438 fixed a comparability bug this branch's numbers were taken across: linopy's `to_highspy()` was being timed *with* variable naming, which neither of our solver sinks does — 82% of its HiGHS hand-off. Every `highs` ratio here was therefore measured against a linopy doing work we do not do, and flatters us. #439 moved the engine again on top of that. So the tables and all three results files go back to main's, and the whole measurement is re-run from a clean tree at a commit this branch can name.
…ug 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
changed the title
FBumann
added a commit
that referenced
this pull request
Sep 14, 2026
…th lanes (#1615) > **Prompt:** Lets work on #441 and #1536 > [!NOTE] > The following content was generated by AI. A test, on both lanes, pinning that a divisor written as a power is named and refused. `p / (d ** 2) <= 1` with `d` missing a row used to split the lanes, and neither answer was usable. The cause was upstream: `program.children` in math-spec had no branch for a power, so `d ** 2` hid `d` from every walk this package asks for a divisor's names. The relational lane named the divisor `''`; the eager lane found no divisor to check, filled the gap with a zero coefficient and built `inf` into the matrix with no refusal at all. That was fixed upstream in [energy-models/mathspec#403](energy-models/mathspec#403) (PR #404, in the pinned `alpha.85`). What this tree owed is the guard — the gap was never in the language's coverage rules, but in what this package asks the language. Both lanes now name `d` and refuse in one identical sentence. Closes #1536. The issue's `blocked:upstream` label is now stale — the blocker merged. <details><summary>What was verified</summary> - New test `test_a_sparse_divisor_written_as_a_power_is_named_and_refused_on_both_lanes` in `tests/test_arithmetic_laws.py`, beside the existing divisor tests, using `both_lanes_refuse` (which builds the two lanes apart, so an eager lane that silently built `inf` would fail the raise rather than pass unnoticed). - Confirmed against a simulated pre-#403 `children` (the `Power` branch dropped): the test fails, so it exercises the gap rather than passing on a broken tree. Against the pinned math-spec it passes; both lanes return: ``` constraint 'c': parameter 'd' is used as a divisor but covers 1 fewer coordinates than it is indexed over. … ``` - `tests/test_arithmetic_laws.py`: **26 passed** (both lanes, linopy from git master). `ruff check` and `ruff format --check` clean. - **Not run:** `pixi run check` — pixi is unavailable here, so the tests and linters were run through a local venv. This is a test-only change, so `src/` and the schema are untouched. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JyxyehhHvXsBqf6NxeQgU1 --- _Generated by [Claude Code](https://claude.ai/code/session_01JyxyehhHvXsBqf6NxeQgU1)_ Co-authored-by: Claude <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
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.
Measured at
98f382d, on a clean tree. Every table comes from one sitting — the ladder, the density sweep and the scaling run — so all three results files describe the same code, and each fingerprint carries that hash. The previously committed one recorded782f76b-dirty, which identifies nothing.The correction that matters
An earlier revision of this PR published
highs-sink numbers taken across the bug #438 fixed. linopy'sto_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. Those numbers flattered us and I had already written them up as wins.highs,lrungdispatchpeakfleetpeakprofiledpeaktransportpeakSo the page's summary goes 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. With both arms holding the same artifact, HiGHS's own dense model dominates and our COO triples are what is left over.The
lpsink barely moved — that comparison was already like for like — and it is where the representation actually pays: ahead on peak on four of six, 0.75× ondispatch, 0.31× onsector.Where we stand,
lrunghighshighslplpprofiledis the case in the ladder we lose, on both sinks. Stated in the prose rather than left in a table.A conclusion still reverses
## Absurd sizesread 1.07× peak at 120M and concluded our memory headroom "is a small-and-sparse-model property, not a scaling one". Re-measured: 0.68–0.78× from 10k to 120M variables, 0.73× at the top.Two things moved it, one of them ours: #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 section now says the old number was drawn across the noise floor — and that it is the LP sink, so it is not evidence for the
highspeak row.What got worse, and is not buried
dispatchbuild is 10–12% slower —maintain_order='left'from perf(engine): cols is positional, and the order is produced rather than repaired #433, bought with −21% peak on the same rung.fleet/highspeak is the one case that got heavier while the rest got lighter, and I have no explanation for it.Not touched
## Sink capabilitiesstill 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.Checks
mkdocs build --strict, docs-site and doc-example tests pass.bench.plotregenerated the chart page from the same run.🤖 Generated with Claude Code