fix(linopy)!: the lane selects v1 on import, and fill= takes the identity of its position - #293
Conversation
…tity of its position
Two corrections to the same mistake, found by auditing for other places we
answer on linopy's behalf.
**The package never opted into v1.** `tests/oracle.py` sets
`options['semantics'] = 'v1'` and argues for it well — measuring against legacy
"would pin this package to the behaviour v1 classifies as a bug". `src/farkas/`
did not, and linopy's default is legacy. So the tested configuration was not the
shipped one, and the two lanes answered the same YAML differently:
masked variable in a product, no test harness
EAGER (as shipped) objective 25.0 x[b] = 0 legacy fills absent with 0
RELATIONAL objective 125.0 x[b] = 100 row dropped, SPEC §6
A wrong answer, not a wrong error, and live since #239 — this commit's shift
work would merely have added a second instance. Set on import: the option is
global, and linopy's own context manager cannot scope it (`__exit__` calls
`reset()`, restoring *all* options to their defaults rather than their prior
values, so it would discard a caller's `display_max_rows`).
The two tests for it run in a **subprocess**. An in-process assertion would pass
whether or not the package sets it, since importing the harness sets it — which
is precisely how this shipped unnoticed.
**`fill=` takes a number because the right fill is positional.** 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. Restricting to the literal 0 got that wrong —
`x * shift(eff, t=1, fill=1)` leaves the first coordinate governed by its own
bound, where `fill=0` pins it, which is the pin this branch exists to remove
wearing the coefficient's hat.
Over data any number is accepted, since it is a data fill both lanes do
natively; the relational lane now writes the rows for a nonzero one, because a
const fragment reads a missing row as zero and `fill=1` exists only if something
puts it there. Over an operand carrying a variable 0 remains the only
representable fill — there a vacated slot contributes no term, and a nonzero one
would be a constant standing where a term was.
BREAKING CHANGE: importing `farkas.linopy` sets `linopy.options['semantics']`
to 'v1' process-wide. Models built through this lane change answer wherever a
variable is masked or shifted — that is the point, and it is the answer the
native engine has always given.
…g it is a release step
|
Warning Review limit reached
Next review available in: 10 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 (7)
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 |
1 participant
These two commits were on #291's branch but not in its squash — #291 merged
fea0d37alone. So the half of that work that makes it correct for users is missing from main, and0.1.0-alpha.12shipped without it.main is currently diverging, measured
#291 made the relational lane drop the row for an acyclic
shiftand removed thevacated()fill from the eager lane. But the package never opts into linopy's v1 convention, and linopy's default islegacy, which zero-fills absent slots. So the eager lane keeps the row the native engine drops:A plain ramp constraint,
x - shift(x, t=1) <= r. 10× apart, no error either side.The suite does not see it because
tests/oracle.py:51setsoptions['semantics'] = 'v1'andsrc/farkas/does not — the tested configuration is not the shipped one. That predates #291 for masked variables (25.0 vs 125.0, live since #239); #291 added theshiftcase on top.1. The lane selects v1 on import
tests/oracle.pyalready argues the case: measuring against legacy "would pin this package to the behaviour v1 classifies as a bug (PyPSA/linopy#712)". The setting belongs where users get it.Set on import, because linopy cannot scope 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. The cost is real and stated in the comment: a process importing this module has its own linopy arithmetic changed too.Both tests run in a subprocess. In-process they would pass whether or not the package sets the option, since importing the harness sets it — which is exactly how this shipped unnoticed. One asserts the option; the other drives both lanes from a clean interpreter and asserts they agree.
2.
fill=takes a number, because the right fill is positionalRestricting it to the literal
0reproduced the very bug #291 exists to remove, one position over. v1 refuses to fill on a caller's behalf for precisely 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, unchanged. A vacated slot contributes no term there, so a nonzero fill would be a constant standing where a term was.3. The dependency floor is now load-bearing
Dropping the version guard means the import does
options['semantics'] = 'v1'unconditionally, andset_valueraisesKeyErrorfor anything lacking it.0.9.0 > 0.8.0.post1.dev0, so a linopy released without v1 would satisfy the floor and crash the import.[tool.uv.sources]does not cover this — 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.Gates: 534 passed, ruff clean, pyrefly 0 errors.