perf(sinks): a solution is a vector, not a vector beside its own index by FBumann · Pull Request #439 · fluxopt/specsolve

FBumann · 2026-07-31T18:18:14Z

Follow-up to #414, using #433's argument on the other side of the solve.

What

#414 made the read-back positional — a declaration owns a contiguous run of
labels, so its share of a solver vector is a slice:

labels.sort(label).select(*dims).with_columns(values['value'].slice(start, height))

The frame the solvers hand back never caught up. _labelled still builds an
arange alongside the values, as the key for the join #414 deleted:

return pl.DataFrame({label: np.arange(count, dtype=np.int64), 'value': ...})

Nothing reads it. Its docstring still explains it as "the join column".

That is 8 bytes a column, held for as long as the Result is — and the same
again in rows for the duals:

rung	columns	dead index
dispatch/l	10.0M	80 MB
fleet/l	12.0M	96 MB
dispatch/2xl	120.0M	960 MB

This is exactly what #433 removed from cols — "the column would be the
frame's own row number" — on the build side. Same argument, other side.

How

Both solvers return a pl.Series of the values; Result holds one;
_read_back slices it directly. The Solve type alias moves with them, which
is what caught the two return annotations I'd missed — pyrefly, not me.

No benchmark

Nothing here is faster: an allocation is simply not made, and its size is
count * 8, which is arithmetic rather than something to measure. The wall-time
effect is whatever np.arange of that size costs, which is not worth claiming.

Test

test_a_solver_hands_back_a_vector_and_not_an_index, over both solvers: the
value is a Series named value, spanning the model. As with #433, the
numbers are identical either way — nothing about the answer can see this
regressing, so it has to be asserted on the shape.

756 tests pass, including the Gurobi ones.

🤖 Generated with Claude Code

Summary by CodeRabbit

Improvements
- Solver results now use streamlined value vectors for primal and dual solutions.
- Solution read-back and Parquet exports continue to support both solution types with improved consistency.
Bug Fixes
- Fixed solver value handling to ensure correct slicing and export of solution data.
Tests
- Added coverage across configured solvers to verify solution types, names, and expected lengths.

#414 made the read-back positional — a declaration owns a contiguous run of labels, so its share of a solver vector is a slice. The frame the solvers hand back never caught up: `_labelled` still builds `arange(count)` alongside the values, as a key for the join #414 deleted. Nothing reads it. That is 8 bytes a column, held for as long as the result is: 80 MB at `dispatch/l`, 96 MB at `fleet/l`, 0.96 GB at the `2xl` rung — and the same again in rows for the duals. It is the argument #433 used to take `col` off `cols` ("the column would be the frame's own row number"), on the other side of the solve. So both solvers return a `pl.Series` of the values, and `Result` holds one. `_read_back` slices it directly instead of reaching through a column name. No benchmark: nothing here is faster, an allocation is simply not made, and its size is `count * 8` rather than something to measure. `test_a_solver_hands_back_a_vector_and_not_an_index` pins it across both solvers, because — as with #433 — the numbers are identical either way and nothing about the answer can see the difference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-07-31T18:18:31Z

Warning

Review limit reached

@FBumann, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4efe3d65-e6a2-44f2-b422-15cfbfbf91a9

📥 Commits

Reviewing files that changed from the base of the PR and between 14ae685 and 3a0fd97.

📒 Files selected for processing (2)

src/lpspec/relational/engines/polars/executor.py
tests/test_relational.py

📝 Walkthrough

Walkthrough

Solver primal and dual outputs now use positional Polars Series values instead of single-column DataFrame objects. Gurobi, HiGHS, execution, result storage, parquet export, and relational tests use the updated representation.

Changes

Solver series handoff

Layer / File(s)	Summary
Solver output contract and adapters `src/lpspec/relational/sinks/solvers/__init__.py`, `src/lpspec/relational/sinks/solvers/gurobi.py`, `src/lpspec/relational/sinks/solvers/highs.py`	Solver return annotations and implementations now produce optional positional `pl.Series` values named `value`.
Execution and result handoff `src/lpspec/relational/engines/polars/executor.py`, `src/lpspec/relational/result.py`	Execution slices solver series directly. `Result` and parquet export accept optional series values.
Cross-solver validation `tests/test_relational.py`	A parameterized test verifies series names, lengths, and primal and dual outputs for each configured solver.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

FBumann/lpspec#189: Directly extends the same Polars solver-result handoff from labeled DataFrame values to positional Series values.
FBumann/lpspec#418: Updates Gurobi and HiGHS result handling in the same solver and executor modules.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: solver solutions now use value-only vectors instead of indexed data frames.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch perf/polars-solution-vector

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.}

coderabbitai

Actionable comments posted: 1

🤖 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 `@src/lpspec/relational/engines/polars/executor.py`:
- Line 388: Validate solver primal and dual vector lengths immediately after the
solver returns and before constructing Result, requiring primal length to equal
self._n_cols and dual length to equal self._n_rows. Raise LpspecError with both
returned and expected lengths when either vector is malformed, preventing
values.slice from hiding trailing omissions. Add regression coverage for short
solver vectors.

🪄 Autofix (Beta)

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: 7a453d9b-db18-4a61-ba6c-ebf4ddae8107

📥 Commits

Reviewing files that changed from the base of the PR and between 782f76b and 14ae685.

📒 Files selected for processing (6)

src/lpspec/relational/engines/polars/executor.py
src/lpspec/relational/result.py
src/lpspec/relational/sinks/solvers/__init__.py
src/lpspec/relational/sinks/solvers/gurobi.py
src/lpspec/relational/sinks/solvers/highs.py
tests/test_relational.py

Reading a solution back is positional, so a vector of the wrong length is an answer about a *different* model rather than a short answer about this one. Refused where the solver hands it over rather than where it is read. The objective comes back from the solver directly, so a `Result` built on a broken vector reports a plausible number and fails only if someone asks for a coordinate — and then from inside polars, about a frame the caller never asked for. A vector that is too *long* was not refused at all: every declaration's slice still lands. `gurobi._duals` already did this for its own half; this is the same check where both solvers pass through, and it is one comparison against a count. Raised by CodeRabbit on #439. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

FBumann · 2026-07-31T18:25:50Z

Verified against the current code before changing anything.

The correctness claim does not hold. values.slice(...) clamps, and
with_columns then refuses the mismatched column — it does not pad or hide a
trailing omission:

vector length vs a 3-row frame
  exact  (3): OK
  short  (2): ShapeError
  empty  (0): ShapeError
  single (1): InvalidOperationError
  long   (4): ShapeError

That was #414's reason for attaching the slice as a column rather than
concatenating it as a frame.

The rest of it is right, for two reasons the finding did not give, so I have
done it in 0664786:

with_columns refuses it in the wrong place. The objective comes back
from the solver directly, so a Result built on a broken vector reports a
plausible number and fails only if someone later asks for a coordinate —
and then with a polars ShapeError about a frame the caller never asked
for. _spanning refuses it at the hand-back, naming the solver, the
quantity and both counts.
A vector that is too long was never refused at all. Every
declaration's slice still lands, so nothing raises anywhere. That case is
the one the suggestion happens to fix and the existing guard genuinely
misses.

gurobi._duals already did this for its own half; the check now sits where
both solvers pass through, and it is one comparison against a count.

Regression coverage is
test_a_solver_vector_that_does_not_span_the_model_is_refused, parametrised
short and long, driven by a monkeypatched solver that truncates or pads the
primal. Verified to fail with the guard removed.

#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.

…-off #439 changed the solver's answer from a `DataFrame` beside its own index to a bare `pl.Series`, read positionally, and added `_spanning` to refuse a vector that does not span the model. Both land on `Engine` here rather than in the polars executor, because `solve` does. The check belongs to the hand-off and both engines hand off through this one, so the duckdb engine gets it without a line of its own — the third time that split has paid out on a feature written after it (gurobi, writer-by-suffix, now this). `Result._executor` stays `Engine` and its value fields become `pl.Series`: the branch's type and main's shape, which is the same resolution the last three merges needed. 780 passed on polars, 779 passed / 5 skipped on duckdb, ruff + pyrefly (0 errors) + mkdocs --strict clean.

FBumann added area:engine Lowering, IR, relational executor, the sink contract perf Wall time, memory, or I/O cost — not a wrong answer engine:polars Specific to the polars engine — the default labels Jul 31, 2026

coderabbitai Bot reviewed Jul 31, 2026

View reviewed changes

Comment thread src/lpspec/relational/engines/polars/executor.py

FBumann force-pushed the perf/polars-solution-vector branch from 0664786 to 3a0fd97 Compare July 31, 2026 18:25

FBumann merged commit 76600ab into main Jul 31, 2026
4 checks passed

fluxopt-release Bot mentioned this pull request Jul 31, 2026

chore(main): release 0.0.1-alpha.57 #442

Merged

Navigation Menu

Repository navigation

perf(sinks): a solution is a vector, not a vector beside its own index - #439

perf(sinks): a solution is a vector, not a vector beside its own index#439
FBumann merged 2 commits into
mainfrom
perf/polars-solution-vector

FBumann commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading

Review limit reached

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

FBumann commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

FBumann commented Jul 31, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

No benchmark

Test

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FBumann commented Jul 31, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

FBumann commented Jul 31, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 31, 2026 •

edited

Loading