feat(compat): answer linopy v1 where the position knows the answer - #236
Conversation
…the answer The oracle lane runs clean under `linopy.options["semantics"] = "v1"`, which issue #8 has been parked on since the tracked PR (PyPSA/linopy#591) was closed unmerged. The live work is PyPSA/linopy#717; its spec is `doc/design/ convention.rst` on `feat/arithmetic-convention`. Two things the convention changes for us, and one it turns out it cannot: **§8 never fires.** Its "shared dimensions must carry identical labels" exists because linopy operands are arbitrary xarray objects with independent indexes, so a mismatch is ambiguous between "different data" and "subset". We resolve a master coordinate per dimension before any data binds (SPEC §8) and reindex every operand to it, so a superset already raises, a subset is filled and a reorder is fixed by label. Measured: four orderings of `a + factor + b` with a sparse factor and a masked variable produce byte-identical rows, so the associativity break v1 §8 exists to stop (PyPSA/linopy#711 — chained left-joins dropping coordinates) has no mechanism here. **§5 does fire, on our central idiom.** `load_parameters` reindexes to the master coordinates, so a parameter covering a subset of its dims arrives at linopy as NaN — and v1 refuses a NaN in a user-supplied constant, because from inside linopy a deliberate absence and a data error are indistinguishable. That is every sparse parameter, which is what SPEC §8 means by "sparse data gives sparse variables" and what a generated binder emits by the tableful. The 414 tests passing beforehand were not evidence otherwise; the suite had no sparse float parameter. It has one now, and it fails without this change. **So the answers are given per position, not once in the loader.** The same missing row means three different things: zero in a coefficient, an error in `bounds:` (unbounded is not bounded-at-zero), false in a `where` (SPEC §6's bare name is "non-null and finite"). A single fill in the loader would have to pick one and be wrong for the other two. `linopy/semantics.py` is where those answers live, mirroring linopy's own module of that name and for its reason: the evaluator stays about evaluating, and a later change to the convention is a single-file diff. All three functions are correct under the legacy convention too — they resolve absence to the value legacy reached implicitly — so none is conditional on the option. What v1 changed is that staying silent stopped being available. Verified on three configurations: released linopy 0.9.0 (415 passed), the v1 branch under `semantics="v1"` (415 passed), and the same branch under `semantics="legacy"` (413 passed, 2 failing only on `LinopySemanticsWarning` promoted to an error by our own filterwarnings). Refs #8
📝 WalkthroughWalkthroughAdds a linopy semantics module for treating missing coefficients, variables, and shifted positions as zero. The builder applies these semantics during AST and helper evaluation, with documentation and a differential regression test for sparse parameter dimensions. ChangesLinopy semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/farkas/linopy/builder.py`:
- Around line 310-311: Update ParameterNode handling in the builder’s evaluator
to preserve missing parameter values by removing coefficient-style zero filling
from semantics.coefficient. Add fillna(0.0) only at expression-coefficient join
points where coefficient evaluation requires it. In tests/test_relational.py
lines 723-731, retain c with t=0 for the bound case and add or adjust separate
coverage for missing bound parameters to verify they raise.
In `@src/farkas/linopy/semantics.py`:
- Around line 39-44: Replace the Any annotations in the exported helpers
coefficient, present, and vacated with narrow protocols or supported public
types describing the required fillna() and to_linexpr() operations and their
concrete return values. Preserve each helper’s existing behavior while ensuring
Pyrefly can type-check all inputs and outputs without widening to Any.
🪄 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: ae80b510-b29a-4784-8df0-dd3cc3bc6ae3
📒 Files selected for processing (4)
ARCHITECTURE.mdsrc/farkas/linopy/builder.pysrc/farkas/linopy/semantics.pytests/test_relational.py
| if isinstance(node, ParameterNode): | ||
| return ctx.dataset[node.name] | ||
| return semantics.coefficient(ctx.dataset[node.name]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)builder\.py$|test_relational\.py$|src/farkas/linopy|tests/test_relational' || true
echo "== builder outline =="
if [ -f src/farkas/linopy/builder.py ]; then
wc -l src/farkas/linopy/builder.py
ast-grep outline src/farkas/linopy/builder.py --view expanded | sed -n '1,220p' || true
echo "== relevant builder lines =="
sed -n '260,340p' src/farkas/linopy/builder.py | cat -n
fi
echo "== test relevant lines =="
if [ -f tests/test_relational.py ]; then
wc -l tests/test_relational.py
sed -n '680,755p' tests/test_relational.py | cat -n
fi
echo "== search parameter/evaluator/coefficient/where/bound terms =="
rg -n "def .*eval|ParameterNode|coefficient|missing|bound|where|SPARSE_COEFFICIENT_MODEL|SPARSE_BOUND|sparse" -S .Repository: FBumann/farkas
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== builder evaluate/_eval helpers =="
sed -n '480,555p' src/farkas/linopy/builder.py | cat -n
sed -n '540,578p' src/farkas/linopy/builder.py | cat -n
echo "== semantics coefficients/missing behavior =="
wc -l src/farkas/linopy/semantics.py
sed -n '1,220p' src/farkas/linopy/semantics.py | cat -n
echo "== test harness and missing bound related tests =="
rg -n "def differential|class differential|SPARSE_COEFFICIENT_MODEL|missing.*bound|bound.*missing|NULL bounds|NaN|null|farkas\.relational|linopy\.extend" tests tests/src 2>/dev/null | sed -n '1,220p' || true
sed -n '1,120p' tests/test_relational.py | cat -n
echo "== relational bound null behavior references with context =="
sed -n '320,345p' src/farkas/relational/executor.py | cat -n
sed -n '648,670p' src/farkas/relational/executor.py | cat -n
sed -n '188,210p' src/farkas/relational/sql.py | cat -n
echo "== focused functional probe of current builder/semantics code via import if available? =="
python3 - <<'PY'
import sys
for m in ['src.farkas.linopy.semantics']:
print("avail", m)
PYRepository: FBumann/farkas
Length of output: 22174
Preserve missing values for parameters in non-coefficient positions.
ParameterNode currently applies coefficient(fillna(0.0)) from the evaluator, so sparse bound data silently becomes <= 0 on missing rows. This path is currently limited to constraint RHS/expression evaluation, but it already conflicts with the relational lane rule that missing bound parameters are NULL and raise. Keep coefficient filling out of semantics.coefficient and add zero-filling only at expression-coefficient join points; this also invalidates the sparse c omission in tests/test_relational.py, which should keep t=0 for the bound while testing missing bound errors separately.
📍 Affects 2 files
src/farkas/linopy/builder.py#L310-L311(this comment)tests/test_relational.py#L723-L731
🤖 Prompt for 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.
In `@src/farkas/linopy/builder.py` around lines 310 - 311, Update ParameterNode
handling in the builder’s evaluator to preserve missing parameter values by
removing coefficient-style zero filling from semantics.coefficient. Add
fillna(0.0) only at expression-coefficient join points where coefficient
evaluation requires it. In tests/test_relational.py lines 723-731, retain c with
t=0 for the bound case and add or adjust separate coverage for missing bound
parameters to verify they raise.
| from typing import Any | ||
|
|
||
| __all__ = ['coefficient', 'present', 'vacated'] | ||
|
|
||
|
|
||
| def coefficient(parameter: Any) -> Any: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep the new semantic helpers type-safe instead of widening to Any.
These exported helpers erase Pyrefly’s type checking at the compatibility boundary. Define narrow protocols for fillna() and to_linexpr() (or use supported public types) rather than using Any for every input and return value.
As per coding guidelines: “Keep Pyrefly strict type checking at zero errors; fix types rather than widening them.”
Also applies to: 64-64, 92-92
🤖 Prompt for 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.
In `@src/farkas/linopy/semantics.py` around lines 39 - 44, Replace the Any
annotations in the exported helpers coefficient, present, and vacated with
narrow protocols or supported public types describing the required fillna() and
to_linexpr() operations and their concrete return values. Preserve each helper’s
existing behavior while ensuring Pyrefly can type-check all inputs and outputs
without widening to Any.
Source: Coding guidelines
Based on
main, independent of #189 — this half of the v1 work touches noengine, so it lands whichever of duckdb or polars wins.
Makes the oracle lane run clean under linopy's v1 arithmetic convention
(PyPSA/linopy#717). No behaviour change on either lane, and no dependency
pin: it converts an inherited legacy accident into a stated intent.
The problem it fixes
load_parametersreindexes every parameter to the master coordinates, so aparameter covering a subset of its dims reaches linopy as NaN. Under v1 that
raises:
It is right to refuse — from inside linopy a deliberate absence and a data error
are indistinguishable. But that is every sparse parameter: SPEC §8's "sparse
data gives sparse variables", and what a generated binder emits by the tableful.
The suite passing beforehand was not evidence against this — it had no sparse
float parameter. It has one now, and it fails without this change.
The shape of the fix
Per position, not once in the loader. The same missing row means three
different things, and only the evaluator knows which:
w * x)semantics.coefficientbounds:whereoperandA single fill in
load_parametershas to pick one and is wrong for the othertwo: an unbounded variable is not a variable bounded at zero, and a mask that
reads true everywhere is not a mask. The loader already made exactly this call
for booleans —
fill_value=False, one line above the float branch.linopy/semantics.pyis the new home, mirroring linopy's own module of thatname and for its stated reason: the evaluator stays about evaluating, and a
later change to the convention is a single-file diff rather than a hunt.
semantics.vacateddoes the same forshift's edge. SPEC §7 declares thatvacated positions contribute zero; v1 §4 counts
.shift()among the operationsthat create absence, so without saying so an acyclic recurrence would silently
lose its first step. Saying it is v1 §7 working as intended — the caller
resolving absence at the call site.
Both are correct under the legacy convention too — they resolve absence to the
value legacy reached implicitly — so neither is conditional on the option. What
v1 changed is that staying silent stopped being available.
Verification
semantics="v1"No pin changed:
to_linexpr().fillna(0)behaves identically across the releasedline and the v1 branch, so nothing here requires depending on an unmerged PR.
Relationship to #234
#234 is the other half, on top of #189: adopting v1's absence semantics as the
language's own (a masked variable's term drops the row rather than contributing
zero), plus
defined(v)and the v1 pin. That half is engine-coupled andbreaking. This one is neither, and is worth landing first either way.
Summary by CodeRabbit
Bug Fixes
Tests