feat!: shift() creates absence — and the lane actually speaks v1 - #291
Conversation
Closes #289, and finishes #8's action item 4 — the last workaround papering over the legacy convention. `shift`'s vacated position used to contribute zero. linopy v1 counts `.shift()` among the four ways to *create* absence, so we were taking the v1 oracle and forcing it back to the legacy answer with `.fillna(0)` — on both lanes, which left the differential harness blind to the difference by construction. It is now absence: it propagates and drops the row, and the eager lane gets that from linopy rather than from us. `shift(x, d=n, fill=0)` is the opt-out, and is deliberately the same escape v1 prescribes for code that wants the older reading. It takes the literal 0 — a nonzero fill is a *constant* at the vacated coordinate, a different fragment kind from a translated term, and refused rather than implemented on one lane only. `roll` refuses it outright, having vacated nothing. **A bare `shift` over a variable-free expression is now a load error.** A parameter has no absence to propagate (a missing row is a zero coefficient, §6), so v1 refuses it and so do we, naming the three things it could have meant. That is what removes `x <= shift(dt, t=1)` silently building `x <= 0` — the pin #288 could only document. Two things this turned up: **Absence has to reach the row, not just the term.** The first cut only dropped the vacated coordinates from the fragment's presence set, which does nothing when presence is `None` — an unmasked variable reports none. The row then survived with the shifted term quietly gone, which is a *different constraint*: `soc - ch == 0` where linopy v1 builds no row at all. Fixed by having an acyclic shift introduce presence rather than only filter it, keyed by `presence_dims` — one column, since a vacated edge lies along one dimension and says nothing about the others. Keying it by the fragment's dims would mean materialising the whole coordinate product to name an edge. **`piecewise:` is why the escape hatch exists.** The nonconvex adjacency row `lam <= seg + shift(seg, bp=1)` needs the zero at the first breakpoint to read `lam <= seg`. Absent, that row drops and the first lambda is bounded only by [0,1] — free to sit on a breakpoint the active segment does not touch, a wrong MILP that still solves. It now emits `fill=0`, and a test asserts the row reaches the model rather than only that the expansion mentions `fill`. `transport_pwl` still matches its published OR-Library optimum. The behaviour change users feel is an acyclic recurrence: `soc` no longer starts from zero, because that was an initial condition the language invented on the modeller's behalf. A model that wants one states it, next to the recurrence under a complementary `where` — which SPEC §2's storage example already did. BREAKING CHANGE: a bare `shift()` now drops the row at the vacated coordinate instead of contributing zero there, and `shift()` over a variable-free expression is a load error. Add `fill=0` for the previous behaviour.
|
Warning Review limit reached
Next review available in: 42 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 (17)
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 |
|
Two corrections pushed, both from auditing for other places we answer on linopy's behalf. The package never opted into v1 — so this PR's title wasn't true for users
A wrong answer rather than a wrong error, and live since #239 — the shift work here would only have added a second instance of it. Scoping it per call was the first instinct and linopy cannot do it: Both tests run in a subprocess, deliberately. An in-process assertion would pass whether or not the package sets the option, because importing the harness sets it — which is exactly how this shipped unnoticed. The second one drives both lanes from a clean interpreter and asserts they agree.
|
FBumann
changed the title
…d it there (#314) Closes #311. `sum(x + y, over=f)` with `y` masked to `f=a` built different models on the two lanes: 400 eager, 240 native — a 40% gap, no error. Addition is fragment concatenation in the relational compiler, so `x + y` is two independent streams and each was reduced over its own coordinates. At row level that is right, since the executor intersects the presences when it assembles the row; a reduction consumes the expression before any row exists, so nothing intersected them. The eager lane was right and I first argued it was not. SPEC §6's carve-out — "a reduction skips what is absent" — has `sum(x, over=d)` over a bare variable as its example, and says a reduction must not propagate absence *outward*. It does not license distributing a compound summand, and distributing is what the relational lane was doing: rewriting `sum(x + y)` into `sum(x) + sum(y)` reads the absent `y[b]` as a zero, which is the reading v1 exists to remove and that #239 and #291 removed from masks and shifts. So reductions are not linear over operands of differing presence. That is the honest consequence of absence being a state rather than a value, and the two spellings are different questions: the total of the net where the net is defined, against the total in minus the total out. SPEC now puts them in a table side by side, because the composition of the two rules was nowhere written down — which is how I came to misread it. `tests/test_arithmetic_laws.py` is the other half. An arithmetic convention is a set of equalities between spellings and nothing else in the suite was checking those: a model can build, solve and agree across both lanes while `a + b` and `b + a` mean different things. Eight laws (commutativity, associativity, subtraction as negated addition, distribution over a variable-free factor and over a divisor, reduction linearity while every operand is total, roll invertibility, and commutativity *under* absence) plus three deliberate non-laws, each asserted through `differential` so a law holding is six numbers agreeing rather than two. The non-laws are the point. This defect is invisible to law-shaped tests — both spellings were individually consistent — and only a test saying "these two are supposed to disagree, and by exactly this much" would have caught it. Writing them also caught one of my own: a "law" that summing over a dim the operand lacks scales by that dim's cardinality. The language refuses that outright at load time. The scaling in `_sum_fragment` applies to a fragment lacking a dim the expression carries, which is not user-visible.
1 participant
Closes #289. Finishes #8's action item 4 — "drop any workarounds that exist to paper over legacy-convention quirks".
Three commits. The second one is the serious one, and it was found by auditing for the first one's general shape: places where we answer on linopy's behalf, so the differential oracle cannot disagree with us.
1. The package never opted into v1 (
89de63a)tests/oracle.py:51setsoptions['semantics'] = 'v1'and argues for it well: measuring against legacy "would pin this package to the behaviour v1 classifies as a bug (PyPSA/linopy#712)".src/farkas/never did, and linopy's default is legacy. The tested configuration was not the shipped one:A wrong answer, not a wrong error, and live since #239 — the shift work below would only have added a second instance. It also made this PR's original title untrue for users, which is why it belongs here rather than in a follow-up.
Set on import. Scoping per call was the first instinct and linopy cannot do it:
with linopy.options(semantics='v1')raisesTypeError(__call__returnsNone), and the only usable form callsreset()on exit — restoring every option to its default rather than its prior value, silently discarding a caller'sdisplay_max_rows. A documented global beats a hand-rolled save/restore around every entry point.Both tests run in a subprocess, deliberately: in-process they would pass whether or not the package sets the option, because importing the harness sets it — which is exactly how this shipped unnoticed.
2.
shift()creates absence (fea0d37)x - shift(x, t=1) <= ry <= shift(dt, t=1)y[0] <= 0— a pinValueErrory <= shift(dt, t=1, fill=0)y[0] <= 0.fillna(0)vacated()was applying.fillna(0)on top of linopy's answer — on both lanes, so the harness had nothing to disagree about. The eager lane now gets absence from linopy itself.A bare
shiftover a variable-free expression is a load error. A parameter has no absence to propagate — a missing row is a zero coefficient (§6) — so v1 refuses it and so do we, naming the three things the author might have meant. Decidable without data, sofk.check()catches it. That removes the pin #288 could only document.Absence has to reach the row, not just the term
The first cut only dropped vacated coordinates from the fragment's presence set, which does nothing when presence is
None— what an unmasked variable reports. The row survived with the term quietly gone:An acyclic shift now introduces presence rather than only filtering it, keyed by a new
presence_dims— one column, since a vacated edge lies along one dimension and says nothing about the others. Keying it by the fragment's dims would materialise the whole coordinate product just to name an edge.piecewise:is why the escape hatch existsThe nonconvex adjacency row is
lam <= seg + shift(seg, bp=1). At the first breakpoint the fill makes itlam <= seg. Absent, that row drops and the first lambda is bounded only by[0,1]— free to sit on a breakpoint the active segment does not touch. A wrong MILP that still solves, in a shipped feature. It now emitsfill=0, and a test asserts the row reaches the model, not merely that the expansion mentionsfill.transport_pwlstill matches its published OR-Library optimum.3.
fill=takes a number, because the right fill is positional (89de63a)Restricting it to the literal
0reproduced the very bug this PR removes, one position over — v1 refuses to fill on a caller's behalf for exactly this reason (convention.rst§7): 0 is the identity of a sum, 1 of a product.Verified identical on both lanes at
fill=1: objective 17.5,x = [10, 5, 2.5].fill=1only exists if something puts it there.0only. A vacated slot contributes no term there, so a nonzero fill would be a constant standing where a term was.rollrefusesfilloutright, having vacated nothing.The behaviour change users feel
An acyclic recurrence no longer starts from zero.
soc == shift(soc, snapshot=1) + charge - dischargehas no row at the first snapshot instead of one assertingsoc[0] == charge[0] - discharge[0]— an initial condition the language was inventing on the modeller's behalf. A model that wants one states it under a complementarywhere, which SPEC §2's storage example already did.Nothing in
examples/usesshift— every shipped model usesroll— so that test is the only model-visible change in the tree.Packaging (
ed50240)Dropping the version guard made the dependency floor load-bearing: the import now does
options['semantics'] = 'v1'unconditionally, and0.9.0 > 0.8.0.post1.dev0, so a linopy released without v1 would satisfy the floor and turn the import into a bareKeyError.[tool.uv.sources]does not help — it resolves for uv, not forpip install farkas[linopy]. Recorded next to the floor so it is raised in the same change that drops the source pin.Docs
fill=escape and its positional identity, and the refusalfillis the one keyword whose key is fixed rather than naming a dimensionports.md— the min-up-time macro sketch gainsfill=0Found on the way, not fixed here
x / dwheredis missing a row: both lanes agree, and both make the row vacuous rather than dividing by zero. The sharpest form of the positional-fill argument — there is no defensible fill for a divisor. Language question, not a lane divergence.DataErrorat build naming the fix; the eager lane builds and dies at solve with linopy's rawValueError: Continuous Variable x contains nan's in field(s) ['upper']. Same verdict, worse timing and message than SPEC §9 asks for.Gates: 534 passed, ruff clean, pyrefly 0 errors,
mkdocs build --strictclean.