fix: a dimensionless parameter is one row, and now has to be - #225
Merged
Conversation
`_check_one_row_per_coordinate` returned early when a parameter had no dims, on the reading that there was no coordinate to group by. But a parameter with no dims has exactly one coordinate — the empty one — so the rule it was skipping is the rule that matters most there. A dimensionless parameter is broadcast by joining on nothing, which is correct for one row and a silent row multiplication for two: duplicate `cols` entries for one variable in a bound, duplicate mask rows in a where. It also left `keyed` asserting at-most-one-row-per-key over a source that had two, which is what turned the duplicate columns into HiGHS refusing the batch and the engine blaming itself in the message. A user who gets here has usually passed a column they thought was indexed, so the message says what a dimensionless parameter means rather than only what is wrong with the source. Zero rows is caught by the same count. Closes #166 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
This was referenced Jul 28, 2026
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>
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
1 participant
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.
The polars-side half of #166. The other half — plus #197 and #198, which are language-layer — is #223 against
main; those files are byte-identical on both branches, so they arrive here on the next rebase and are deliberately not duplicated in this PR.What was wrong
_check_one_row_per_coordinatereturned early when a parameter had no dims, on the reading that there was no coordinate to group by. But a parameter with no dims has exactly one coordinate — the empty one — so the rule it was skipping is the rule that matters most there.A dimensionless parameter is broadcast by joining on nothing: correct for one row, a silent row multiplication for two. Duplicate
colsentries for one variable in a bound, duplicate mask rows in a where.On this branch it also left
keyedasserting at-most-one-row-per-key over a source that had two. That is what turned the duplicate columns into HiGHS refusing the batch, and the engine blaming itself:A user who gets here has usually passed a column they thought was indexed, so the message says what a dimensionless parameter means rather than only what is wrong with the source. Zero rows is caught by the same count.
Cost is one
pl.len()over the source frame, per scalar parameter.Verification
403 passed, 1 xfailed;ruff checkclean;pyrefly0 errors. Tests cover 2 rows, 0 rows, and the one-row control that must still build.🤖 Generated with Claude Code