docs(spec): three behaviours a consumer could only reach by probing - #288
Merged
Conversation
A downstream port (#214) worked around all three, having established each by probing rather than by reading. This states them, and pins each with a differential test — an unrun claim in a language reference is the same shape of thing as an unrun example. #224 filed three items; one of them had already been overtaken by the engine. **Term absence** was item 1, and it is now half wrong: #239 landed absence-propagates hours after the issue, so a masked *variable* drops the row and the trap it describes is gone. The other half is live and unstated — the same expression with the *parameter* missing keeps the row and builds `x <= 0`, which is exactly the silently-wrong shape the v1 convention removed from the variable side. §6 already used `x - rel_max * size <= 0` as its example of what does *not* happen; a reader who stops there concludes they are safe. It now says which operand decides that, and that a coefficient table sparse because the *component* is absent wants a mask. **shift/roll over parameters** — §7's `array` covers it and every example took a variable, so the port shipped `dt_previous`, a hand-shifted copy of a table it already had, and deleted it once it knew. The vacated-position zero is the trap in RHS position: `x <= shift(dt, t=1)` pins the first coordinate rather than leaving it free, so `roll` is named as the contrast. **Pinning with equal bounds** is how a quantity that is data in one model and a decision in another stays one declaration, instead of a block per regime with coefficients whose names encode the regime. §2, next to bounds, with the two limits that bite: affinity is unchanged, and a pinned variable cannot bound another. Tests are `differential(..., lp=True)`, so each is asserted on both lanes and the LP re-solve. The parameter-absence one sits beside the benign case it is easy to mistake for — there the zero lands on a coefficient *and* the RHS, so the row constrains nothing; here the RHS is a literal 0. Refs #224
📝 WalkthroughWalkthroughThe specification clarifies equal-bound pinning, sparse coefficient masking, and parameter ChangesRelational semantics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@tests/test_relational.py`:
- Around line 826-827: Update the sparse-test fixtures in the differential()
inputs to use Polars DataFrames instead of pandas Series, including the
omitted-coordinate cases and the existing fixtures near relmax/cost, line 867,
and lines 898-900. Preserve the same column names, index coordinates, and values
so the sparse behavior remains unchanged while exercising the Polars-native
path.
🪄 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: de0b4707-6590-44f2-8a2c-b4ad5cb30494
📒 Files selected for processing (2)
docs/SPEC.mdtests/test_relational.py
Comment on lines
+826
to
+827
| 'relmax': pd.Series({'a': 0.5}), # no row at 'b' | ||
| 'cost': pd.Series({'a': 1.0, 'b': 1.0}), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline tests/differential.py --items all
rg -n -C 3 --glob '*.py' 'def tidy_sources|pl\.(DataFrame|Series)' src testsRepository: FBumann/farkas
Length of output: 30323
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== tests/differential.py lines 70-135"
sed -n '70,135p' tests/differential.py
echo
echo "== tests/test_relational.py lines 800-910"
sed -n '800,910p' tests/test_relational.py
echo
echo "== src/farkas/sources.py tidy_sources"
sed -n '1,220p' src/farkas/sources.py
echo
echo "== src/farkas/relational/frames.py"
sed -n '1,130p' src/farkas/relational/frames.pyRepository: FBumann/farkas
Length of output: 15415
Keep sparse-test fixtures in Polars.
differential() feeds data into both the linopy oracle and tidy_sources(), so these pd.Series inputs use the pandas bridge path instead of Polars-native tables. Replace them with Polars DataFrames for the omitted-coordinate cases as well as the existing pandas fixtures at 826-827, 867, and 898-900.
🤖 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 `@tests/test_relational.py` around lines 826 - 827, Update the sparse-test
fixtures in the differential() inputs to use Polars DataFrames instead of pandas
Series, including the omitted-coordinate cases and the existing fixtures near
relmax/cost, line 867, and lines 898-900. Preserve the same column names, index
coordinates, and values so the sparse behavior remains unchanged while
exercising the Polars-native path.
Source: Coding guidelines
FBumann
added a commit
that referenced
this pull request
Jul 29, 2026
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.
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.
Closes the live part of #224, and reports the part of it that the engine overtook.
One of the three items is stale, and that is the interesting one
#224 was filed 2026-07-28 08:09 UTC.
5eb5943 feat!: absence propagates and drops the row, plus defined(v) (#239)landed about three hours later, which makes the issue's item 1 half wrong: a term whose variable is masked now takes the row with it, so the trap it describes —x - relmax * size <= 0silently becomingx <= 0— no longer exists on that path. SPEC §6 already documents the new semantics.The other half is live and was not stated. Change one operand and the hazard returns intact:
Absence is a property of variables, so nothing propagates — the row survives and the term that was the bound is dropped. This is correct, it is the documented reading of a sparse coefficient table, and it is the same silently-wrong shape the v1 convention removed from the variable side. §6 uses that exact expression as its example of what does not happen, so a reader who stops there concludes they are safe on both operands. It now says which operand decides it.
The other two, verified rather than assumed
shift/rollread parameters. §7'sarraycovers it by the letter and every example took a variable, so the downstream port shippeddt_previous— a hand-shifted copy of a table it already had — and deleted two parameters once probing revealedshift(dt, t=1)works. The vacated-position zero is the trap in RHS position, where it is a bound and not a term:t=0t=1t=2x <= shift(dt, t=1)x <= 0— a pin, not "unconstrained"x <= 5x <= 6x <= roll(dt, t=1)x <= 7x <= 5x <= 6Pinning with equal bounds. How a quantity that is data in one model and a decision in another stays one declaration, instead of a block per regime with pre-multiplied coefficients named after the regime rather than the quantity. Verified both regimes out of one equation: pinned at 10 gives
rate <= 8, free to 50 givesrate <= 40. Documented in §2 next to bounds, with the two limits that bite — affinity is unchanged (size * onis still refused), and a pinned variable cannot bound another.Each claim is pinned by a test
Three docs claims with nothing asserting them is the rot this repo already spends
test_doc_examples.pyon. All three go throughdifferential(..., lp=True), so each is checked on both lanes and the LP re-solve.The parameter-absence test sits directly beside
test_a_parameter_covering_a_subset_of_its_dims_means_zero_on_both_lanes, which is the benign case it is easy to mistake for: there the zero lands on a coefficient and a right-hand side, so the row constrains nothing; here the right-hand side is a literal0and the missing coefficient was the whole bound.Not in this PR
Gates: 525 passed, ruff clean, pyrefly 0 errors,
mkdocs build --strictclean.Summary by CodeRabbit
Documentation
shiftandrollbehavior for parameters and variables, including zero-filled boundaries and cyclic horizons.Tests