Skip to content

test(bench): carry over the profiled case and the sink axis, and re-measure on both - #215

Merged
FBumann merged 4 commits into
polars-enginefrom
bench/carry-over-profiled-and-sinks
Jul 28, 2026
Merged

test(bench): carry over the profiled case and the sink axis, and re-measure on both#215
FBumann merged 4 commits into
polars-enginefrom
bench/carry-over-profiled-and-sinks

Conversation

@FBumann

@FBumann FBumann commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Carries the two ideas from #205 onto polars-engine, and re-measures everything on them. Based on polars-engine after #211.

#205 does not cherry-pick. It also carries --memory-limits, --chunk-rows and workdir_bytes — the duckdb budget apparatus #189 deleted — so taking the commit would have reintroduced it. Both ideas are ported by hand instead.

The headline: the sink decides the answer

transport's peak is 1.62x through the LP writer and 0.78x through the hand-off. Same build, same rung. This file has called that row "the one open number at scale" — on the sink most callers actually use, we are ahead of linopy on it.

l rung, wall / peak:

lp highs
dispatch 0.72x / 1.03x 0.36x / 0.95x
nodal 0.63x / 0.83x 0.32x / 0.80x
sector 0.37x / 0.32x 0.25x / 0.32x
transport 0.86x / 1.62x 0.46x / 0.78x
profiled 1.74x / 1.14x 1.13x / 0.94x

Ahead on both axes on four of five cases through the hand-off. The LP path spends its peak turning doubles into text; HiGHS's own dense model is resident in both arms and dwarfs the difference between the lanes that filled it. The "what this says" section now reads per sink, because the two disagree by more than the noise between them.

What is in it

profiled — the one case whose input is the same order as its model: availability dense over (snapshot, node, tech), a 12M-row table against a 12M coordinate product at l. It is the shape the eager lane handles best, and it is the case we lose (1.74x LP wall). That is why it is here — a ladder holding only shapes that suit one engine proves nothing.

--sinks lp highs, both by default, and build_highs split out of solve_direct to give it a seam that stops before run(). This is what makes the build-and-hand-off section reproducible: it was a hand-made table with no script behind it, carrying a note from me saying its peak column was stale and could not be re-run without one. That note and that table are both gone.

Two things it found

A real bug. profiled writes its dim columns dictionary-encoded — what pandas does for any Categorical, and what any writer does for a 12M-row table of repeated labels. polars will not join Categorical against String, and dim frames are plain, so binding such a source died with a schema error from inside a join. duckdb coerced silently, so this arrived with #189 rather than being found by it. Source dim columns are now cast at bind time; the test fails without it.

The density sweep no longer refuses its own claim. Peak improves at every rung now (0.87x → 0.74x) where linopy used to win the sparsest one. Not because the prediction changed but because a mask got cheaper for us — #212 made masked labels arithmetic, and the sweep had been measuring our own cost of being sparse.

Provenance

294 records, five cases, two sinks, both arms, best of three, one invocation, parity gate green on all five at 0.0e+00 relative. Machine quiet. 401 passed / 1 xfailed, ruff and pyrefly clean.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 12125b36-20c8-4f07-a555-6ea0d69e1df9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bench/carry-over-profiled-and-sinks

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

FBumann and others added 4 commits July 28, 2026 10:12
A `Categorical` dim column is a source encoding, not a different model. pandas
writes one for any `Categorical`, and any writer will dictionary-encode a
12M-row table of repeated node names. polars will not join `Categorical`
against `String`, and the dim frames are built from declared coordinate values
and are plain — so binding such a source failed with a schema error from
inside a join, a long way from anything a caller can act on.

Source dim columns are cast to `String` at bind time. Casting the source side
rather than the dim side is deliberate: the dim frame is the authority on what
a coordinate is, and a source is whatever a caller happened to hand over.

duckdb coerced this silently, so it arrived with the polars rewrite rather
than being found by it. Found by the `profiled` bench case, which writes its
dim columns exactly this way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hand-off without the simplex. `solve_direct` is now this plus `run()`, and
the seam exists because the two are different questions: the simplex is the
same work whoever filled the model, so a measurement that includes it says
nothing about the lane that filled it.

`bench/` ends here, and linopy's `Model.to_highspy()` is the same seam on that
side of the fence — which is the only reason the two arms are comparable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e it on

Ports the two ideas from #205, which was written against the duckdb harness
and cannot cherry-pick: that commit also carries `--memory-limits`,
`--chunk-rows` and `workdir_bytes`, the budget apparatus #189 deleted.

`profiled` is the one case whose input is the same order as its model. Every
other case builds its coordinate product from parameters far smaller than the
product they explode into, so reading the parquet is noise; here
`availability` is dense over `(snapshot, node, tech)`, which at the `l` rung is
a 12M-row table against a 12M coordinate product. It is also the shape the
eager lane handles best, and the case we lose — which is the point, because a
ladder holding only shapes that suit one engine proves nothing.

`--sinks lp highs`, both by default. The LP file is the artifact fewest callers
want, and it is not the same comparison: HiGHS's own dense model is resident in
both arms and narrows the gap. It is also what makes the build-and-hand-off
table reproducible, where before it was hand-made with no script behind it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…swer

Full ladder on a quiet machine: five cases, two sinks, both arms, best of
three, parity gate green on all five at 0.0e+00. 294 records, no failures.

`transport`'s peak is 1.62x through the LP writer and 0.78x through the
hand-off. Same build, same rung — the LP path spends its peak turning doubles
into text, while HiGHS's own dense model is resident in both arms and dwarfs
the difference between the lanes that filled it. This file has called that row
the one open problem at scale; on the sink most callers use, we are ahead of
linopy on it. "What this says" now reads per sink, because the two disagree by
more than the noise between them.

The density sweep no longer refuses its own claim: peak improves at every rung
(0.87x -> 0.74x) where linopy used to win the sparsest one. Not because the
prediction changed but because a mask got cheaper for us — #212 made masked
labels arithmetic, and the sweep had been measuring our own cost of being
sparse.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@FBumann
FBumann force-pushed the bench/carry-over-profiled-and-sinks branch from e273c5b to dac907e Compare July 28, 2026 08:13
@FBumann FBumann changed the title bench: carry over the profiled case and the sink axis, and re-measure on both test(bench): carry over the profiled case and the sink axis, and re-measure on both Jul 28, 2026
@FBumann

FBumann commented Jul 28, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto polars-engine (picking up #225), and CI fixed.

Three things were wrong:

  • ruff format --check — I ran ruff check but not ruff format on the final state, and ruff check --fix had reintroduced two files' worth of drift after my last format pass. Fixed.
  • PR title type. bench: is not in the allowed set (feat, fix, perf, refactor, docs, chore, test, ci, build, style, revert). Retitled to test(bench):.
  • The branch was behind.

While fixing the title I split the commits rather than just renaming them, because the original lumped a user-facing bug fix under a bench: subject — it would have landed in the changelog as harness work, or not at all:

fix: a dictionary-encoded source column binds like a plain one
refactor(sinks): split build_highs out of solve_direct
test(bench): a case where I/O is not noise, and a sink axis to measure it on
docs(bench): re-measure on both sinks, and let the sink decide the answer

Each one is independently green (404 passed / 1 xfailed at every commit), so the history bisects.

@FBumann
FBumann merged commit f5af03c into polars-engine Jul 28, 2026
3 checks passed
FBumann added a commit that referenced this pull request Jul 28, 2026
main gained the three duckdb perf PRs (#152, #174, #178), two language-level
fixes (#201, #223) and the bench harness from #205. Resolved by lane:

**Engine files take the polars side.** #152, #174 and #178 are `ROW_NUMBER()
OVER`, `CREATE TYPE ... AS ENUM` and `CREATE TABLE surv_*` against the engine
this branch deletes. Their *ideas* are already here in polars form — #152's as
`_positional`, #174's as #211, #178's as #212 — so taking main's side would
have re-added SQL against a connection that no longer exists.

**Language and validation take main's.** #223's `lowering.py`,
`validation.py` and `linopy/builder.py` changes merged clean and are engine
independent; its executor half and #201's are already here (as #225 and
`_check_one_row_per_coordinate`). `test_objective.py` and `test_validation.py`
arrived whole and pass on this lane unmodified — 404 tests to 414.

**bench takes ours**, which carries #205's two ideas ported rather than
merged (#215): the duckdb commit also brings `--memory-limits`, `--chunk-rows`
and `workdir_bytes`, the budget apparatus this branch removed.

Two tests were combined rather than picked. `test_group_sum`'s docstring says
both true things now — the divergence between lanes, and the aggregate-skip
premise that rests on it. `test_milp` keeps the polars Enum test and gains
main's ragged-chunk test, which is API-level and applies to this sink too.

Main's duckdb label tests are dropped, not ported: they drive `DuckdbExecutor`
and `ex._con` directly. The properties they pin are covered here by
`test_a_mask_that_removes_nothing_labels_exactly_like_no_mask` and
`test_a_factored_mask_labels_exactly_like_the_counted_path`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant