Conversation
📝 WalkthroughWalkthroughThe pull request revises the language specification, architecture rules, expressive ceiling, roadmap, Python API documentation, linopy integration documentation, navigation, and documentation example coverage. ChangesDocumentation contracts and navigation
Estimated code review effort: 2 (Simple) | ~10 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 |
FBumann
changed the title
FBumann
marked this pull request as ready for review
July 31, 2026 10:04
An aggressive pass on length. The principle is the project's own: reference pages carry rules, design notes carry arguments, and the roadmap is an index while the issues carry the argument. ROADMAP loses a third. Everything cut is now in an issue that says it better — the window family in #380/#384, the seam in #381, warm starts in #382, semi-continuous in #383, degree in #261/#84, Track 4 in #89. What stays is the tables, the decisions and the links. SPEC loses section 10 entirely, to docs/api.md. The page's own header says it documents "what a YAML file may contain and what it means", and how to call solve() is not that: nothing on that page changes what a file means. Section 10 is now a three-line pointer, which leaves section 11's number alone and so leaves every inbound link alone. api.md joins the nav under Reference, and TRACKED in tests/test_doc_examples.py — moving the examples out of SPEC dropped six tests until that list caught up, which is the one real hazard in a move like this. ARCHITECTURE states the label contract once instead of three times. It was spread over three paragraphs that each re-asserted var_label is the solver column index; they are now one block that also covers why appending rows is safe. Hard rules 1-5 keep every rule and lose the retelling. One precision fix rides along: the Thesis said the plan is "executed relationally under a fixed memory budget", while Track 5 says plainly that a declared ceiling is not something we have. SPEC 599->527, ARCHITECTURE 532->512, ROADMAP 353->259, plus api.md at 83. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs said two contradictory things. The degree axis section described quadratic as sequenced work with a revisit condition, while the ceiling listed "degree 1 (affine)" as one of three admissibility rules and the non-primitives table listed quadratic as banned. A reader could not tell which was the position, because both were. The position is: the ceiling is **relational ∩ local** and protects streaming. Degree is not part of it — nothing about `variable × variable` is non-relational or non-local, and a coordinate-aligned product is a pointwise self-join. Degree 1 is where the language is, not where it must stay. What actually gates quadratic is what a sink can ingest, which is the capability axis, and the same is true of SOS, indicator and semi-continuous. So quadratic, SOS/indicator and semi-continuous leave "deliberate non-primitives" and become Track 3, ordered by effort behind the capability model: semi-continuous is blocked on nothing, SOS on the capability model, quadratic on that plus a second solver. Cumulative sums over a variable were already moved from banned to priced; the refusals table now holds only what is actually refused — data prep, domain helpers, a variable divisor, structure inside one plan, and a Python modeling API. The tracks renumber as a result (sink capabilities 4 -> 3, memory 5 -> 4), so four inbound anchors move with them, and the link text moves too rather than naming a track that no longer has that number. Two stale references found while editing: ceiling.md cited "rule 6" twice for the no-Python-modeling-API rule, but ARCHITECTURE's hard rules run 0-5 and it is rule 5; and a Track 2c anchor pointed at a heading that no longer exists. ROADMAP 259 -> 200. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three different relationships to linopy were being explained in fragments across every page: it is not a runtime dependency, it is the differential oracle, and it is an opt-in shim for models already in memory. Conflating them is what made "the lanes" read as a structural theme rather than an implementation detail. docs/design/linopy.md now holds all three, and the rest stops mentioning it. SPEC loses 31 mentions down to two pointers. Most were internal guarantees dressed as language rules — "both lanes reject it at load time", "cannot make the lanes diverge", "what makes both lanes agree by construction". Those state that our two implementations agree, which is a correctness property of ours and not a rule about what a YAML file may contain; the reference now just states the rule. The rest explained our semantics by pointing at linopy — "as in linopy.Model.add_variables", "mirroring add_piecewise_formulation", "linopy's v1 arithmetic convention" — which tells a reader who has never used linopy nothing, and in every case the rule was already stated completely in the table above it. Section 8 keeps only the product path. The two paths genuinely accept different *data* inputs (#60) while accepting the same language, and interleaving them normalised the wart; that table moves to the linopy page where it reads as the open question it is. The pitch drops it too: README and index sold the benchmark as "against the eager lane's own best path", which asks a prospective user to know there are two before they know there is one. It is measured against linopy, so it now says so. ARCHITECTURE keeps every mention. The two-lane structure *is* the architecture there — hard rule 3, the four fences, the directory each lives in, all enforced by tests. That page's subject is internal structure. Filed while writing it: LinopyYamlError roots the public exception tree, so the only place a user of the pure-polars path meets the word "linopy" in normal use is the traceback of every error they hit (#389). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FBumann
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs/api.md`:
- Around line 34-35: Update the result-lifetime sentence near “Nothing has to be
released” to use grammatical wording that clearly states the built model owns
the frames, while preserving the existing claim that primal and to_* readers
remain valid for the Result’s lifetime.
- Around line 3-6: Update the opening API overview sentence in the documentation
to replace “this page is the sixteen names” with “this page lists the sixteen
names,” preserving the surrounding wording.
In `@docs/design/ceiling.md`:
- Around line 31-51: Update the degree references in ARCHITECTURE.md, including
the ceiling description and the AST diagram, so they no longer present degree 1
as an admission rule or ceiling clause. Align both references with the
relational/local streamability rules and the sink-capability-based quadratic
boundary described in ceiling.md.
In `@docs/design/linopy.md`:
- Around line 57-70: Clarify the contracts in docs/design/linopy.md lines 57-70:
describe build as creating and returning a new linopy.Model, extend as applying
YAML to and mutating the supplied model in place, and remove or redefine “Both
are pure producers.” Also update docs/api.md lines 64-67 so the existing-model
requirement applies only to extend, not build.
In `@docs/ROADMAP.md`:
- Around line 44-45: Remove the duplicate semi-continuous variables roadmap
entry from Track 1’s item list, or make the Track 3 capability explicitly
reference that existing Track 1 item. Ensure the roadmap contains one status and
one delivery sequence for semi_continuous, covering the entries around the Track
1 and Track 3 sections.
In `@docs/SPEC.md`:
- Around line 440-446: Clarify in the specification’s precedence list that
`sources` and `coords=` are loader/API inputs rather than YAML keys. Update the
entries describing these two inputs while preserving the existing precedence and
value-resolution behavior, so readers do not add them to the closed YAML schema.
- Around line 405-427: Update the opening paragraph describing shift’s accepted
nodes to explicitly qualify that variable-free expressions are not valid with a
bare edge. State that such parameter shifts require an explicit edge value,
masking via where, or edge=wrap, while preserving the subsequent four edge
rules.
- Around line 235-236: Update the `**` operator documentation in §0, §5, and §7
of SPEC.md to distinguish degree-1 language use from Track 3 quadratic support:
reject it only in degree-1 contexts, while documenting Track 3 syntax and/or
linking the sink capability gate that permits quadratic use. Remove wording that
presents `**` as universally refused, and keep the stated load-time rejection
behavior for unsupported contexts.
- Around line 387-388: Clarify the load-error rule in the surrounding
documentation so it applies only to an undeclared standalone where atom of the
form “where: "name"”, while preserving the preceding rule that permits
undeclared names used as RHS string coordinates. Update the sentence near the
bare-name and mask-dimension rules without changing unrelated semantics.
🪄 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: 9af69e0a-f97a-400a-b9f9-afcfb65080ac
📒 Files selected for processing (12)
README.mddocs/ARCHITECTURE.mddocs/ROADMAP.mddocs/SPEC.mddocs/api.mddocs/benchmarks.mddocs/design/ceiling.mddocs/design/linopy.mddocs/guide.mddocs/index.mdmkdocs.ymltests/test_doc_examples.py
Comment on lines
+3
to
+6
| How you *run* a model. The model itself is the YAML file — | ||
| [SPEC](SPEC.md) is what it may contain and what it means; this page is the | ||
| sixteen names that load, check, build, solve and read one back. The surface is | ||
| pinned by a test and the reasoning behind its size is |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the opening grammar.
Change “this page is the sixteen names” to “this page lists the sixteen names”. The current API overview is ungrammatical.
Proposed wording
-this page is the
-sixteen names that load, check, build, solve and read one back. The surface is
+this page lists the sixteen names used to load, check, build, solve, and read a
+model. The surface is📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| How you *run* a model. The model itself is the YAML file — | |
| [SPEC](SPEC.md) is what it may contain and what it means; this page is the | |
| sixteen names that load, check, build, solve and read one back. The surface is | |
| pinned by a test and the reasoning behind its size is | |
| How you *run* a model. The model itself is the YAML file — | |
| [SPEC](SPEC.md) is what it may contain and what it means; this page lists the | |
| sixteen names used to load, check, build, solve, and read a | |
| model. The surface is |
🤖 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 `@docs/api.md` around lines 3 - 6, Update the opening API overview sentence in
the documentation to replace “this page is the sixteen names” with “this page
lists the sixteen names,” preserving the surrounding wording.
Comment on lines
+34
to
+35
| **Nothing has to be released.** The built model is frames this process owns, so | ||
| `primal` and the `to_*` readers stay valid for as long as the `Result` does. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the result-lifetime sentence.
Line 34 says “The built model is frames this process owns”. Replace it with a grammatical statement that clearly identifies the owned frames.
Proposed wording
-The built model is frames this process owns, so
+The built model consists of frames owned by this process, so📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **Nothing has to be released.** The built model is frames this process owns, so | |
| `primal` and the `to_*` readers stay valid for as long as the `Result` does. | |
| **Nothing has to be released.** The built model consists of frames owned by this process, so | |
| `primal` and the `to_*` readers stay valid for as long as the `Result` does. |
🤖 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 `@docs/api.md` around lines 34 - 35, Update the result-lifetime sentence near
“Nothing has to be released” to use grammatical wording that clearly states the
built model owns the frames, while preserving the existing claim that primal and
to_* readers remain valid for the Result’s lifetime.
Comment on lines
+31
to
+51
| A candidate primitive is admissible iff it is **relational** — filter / join / | ||
| group-by-aggregate over tidy tables — and **local**, meaning *pointwise* or | ||
| *bounded-halo*, which compose under partition-wise execution where *global* | ||
| operators do not. Locality is judged in **data space**: reductions over a | ||
| *coordinate* space ("the last snapshot") read only the small, already | ||
| materialised dim tables and stay admissible even though they look global. | ||
|
|
||
| **Read the verdict off the plan.** Rules 2 and 3 are one question asked twice, | ||
| and the compiler already answers it — write the candidate's query over the term | ||
| stream first and read `.explain()`: | ||
| **Degree is not the third rule**, and stating it as one was a mistake this page | ||
| made for a while. Nothing about `variable × variable` is non-relational or | ||
| non-local — a coordinate-aligned product is a pointwise self-join. Degree 1 is | ||
| where the language *is*, not where it must stay: what actually gates quadratic is | ||
| **what a sink can ingest**, which is the second axis below, and the sequence is | ||
| [ROADMAP Track 3](../ROADMAP.md#track-3--capabilities-and-the-degree-line). The | ||
| same is true of SOS, indicator and semi-continuous. Read this page as the | ||
| *streamability* closure and nothing more. | ||
|
|
||
| | Shape of the emitted query | Locality | Rules 2–3 | | ||
| **Read the verdict off the plan.** Relational and local are one question asked | ||
| twice, and the compiler already answers it — write the candidate's query over | ||
| the term stream first and read `.explain()`: | ||
|
|
||
| | Shape of the emitted query | Locality | Admissible? | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the architecture references with the new ceiling.
The new text removes degree from the ceiling. docs/ARCHITECTURE.md still describes degree 1 as the ceiling's first clause at Line 376 and shows degree 1 in the AST diagram at Line 55. Update those references before merge. Otherwise, the architecture and ceiling define different admission rules.
Also applies to: 62-68
🤖 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 `@docs/design/ceiling.md` around lines 31 - 51, Update the degree references in
ARCHITECTURE.md, including the ceiling description and the AST diagram, so they
no longer present degree 1 as an admission rule or ceiling clause. Align both
references with the relational/local streamability rules and the
sink-capability-based quadratic boundary described in ceiling.md.
Comment on lines
+57
to
+70
| For math that belongs on a `linopy.Model` **already in memory** — a PyPSA | ||
| network, say, where the model is built by something else and you want to add | ||
| declared constraints to it. | ||
|
|
||
| ```python | ||
| from lpspec import linopy as lpspec_linopy | ||
|
|
||
| m = lpspec_linopy.build('model.yaml', data={...}, coords={...}) # -> linopy.Model | ||
| lpspec_linopy.extend(m, 'ramp.yaml', data={...}) # mutates m in place | ||
| ``` | ||
|
|
||
| Both are *pure producers*: YAML in, model out, nothing retained. `build` returns | ||
| a plain `linopy.Model` — no accessor, no attached schema, no patched attributes | ||
| — so nothing is lost across `pickle`, `deepcopy` or `to_netcdf`. To inspect the |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the build and extend contracts consistent.
The documentation currently conflates the two entry points. build creates a new linopy.Model; extend applies YAML to an existing model and mutates it in place. State this distinction in both pages.
docs/design/linopy.md#L57-L70: describebuildas creating a model andextendas mutating the supplied model. Remove or redefine “Both are pure producers”.docs/api.md#L64-L67: describe the existing-model requirement only forextend.
📍 Affects 2 files
docs/design/linopy.md#L57-L70(this comment)docs/api.md#L64-L67
🤖 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 `@docs/design/linopy.md` around lines 57 - 70, Clarify the contracts in
docs/design/linopy.md lines 57-70: describe build as creating and returning a
new linopy.Model, extend as applying YAML to and mutating the supplied model in
place, and remove or redefine “Both are pure producers.” Also update docs/api.md
lines 64-67 so the existing-model requirement applies only to extend, not build.
Comment on lines
+44
to
+45
| | 10 | `cumsum`, data only | `cumsum(p, over=dim)`, variable-free | investment schedules, cumulative budgets | coordinate-space | [#384](https://github.com/FBumann/lpspec/issues/384) | | ||
| | 11 | Semi-continuous variables | `semi_continuous: true` | "off, or between min-stable and max", without the big-M pair | n/a | [#383](https://github.com/FBumann/lpspec/issues/383) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep semi-continuous variables in one roadmap track.
semi_continuous appears as Track 1 item 11 at Line 45 and again as a Track 3 capability at Line 120. The PR policy says it moves to Track 3. Remove it from Track 1, or change Track 3 to reference Track 1. Keep one status and one delivery sequence.
Also applies to: 118-122
🤖 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 `@docs/ROADMAP.md` around lines 44 - 45, Remove the duplicate semi-continuous
variables roadmap entry from Track 1’s item list, or make the Track 3 capability
explicitly reference that existing Track 1 item. Ensure the roadmap contains one
status and one delivery sequence for semi_continuous, covering the entries
around the Track 1 and Track 3 sections.
Comment on lines
+235
to
+236
| than a sum. **`**` parses but is not in the language**: it is rejected at load time, so the | ||
| refusal can name the operator and its rewrite. A variable base |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'quadratic|\*\*|degree|Track 3|refused|capabilit' \
docs/SPEC.md docs/design/ceiling.md docs/ROADMAP.mdRepository: FBumann/lpspec
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Roadmap Track 3 section"
sed -n '96,220p' docs/ROADMAP.md
echo
echo "## SPEC degree/operator/reject section"
sed -n '220,250p' docs/SPEC.md
echo
echo "## All SPEC mentions of Track 3/sink capability/quadratic/degree"
rg -n 'Track 3|capacity|quadratic|hessian|Hessian|degree|sink|capability|degree 1|refused|`**`|\*\*' docs/SPEC.mdRepository: FBumann/lpspec
Length of output: 25907
🏁 Script executed:
# Also look at referenced Roadmap section and possible TRACK/degree code for context.
rg -n -C 3 'Track 3|quadratic|degree|Hessian|sink|capability|check|degree.py' README.md docs architecture language tests specs 2>/dev/null || trueRepository: FBumann/lpspec
Length of output: 50373
Align the ** entry with Track 3’s quadratic plan.
docs/SPEC.md currently says ** is refused, but Track 3 plans quadratic not as a ceiling refusal. Update §0/§5 and §7 to say ** is refused only for degree-1 language use, and record Track 3 syntax or link to the sink capability gate.
🤖 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 `@docs/SPEC.md` around lines 235 - 236, Update the `**` operator documentation
in §0, §5, and §7 of SPEC.md to distinguish degree-1 language use from Track 3
quadratic support: reject it only in degree-1 contexts, while documenting Track
3 syntax and/or linking the sink capability gate that permits quadratic use.
Remove wording that presents `**` as universally refused, and keep the stated
load-time rejection behavior for unsupported contexts.
Comment on lines
+387
to
+388
| everything out. An undeclared *bare* name is a load error, and a mask dim outside `foreach` is | ||
| one too (§5.2). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Distinguish standalone names from RHS string names.
The preceding rule allows an undeclared RHS name as a string coordinate. This sentence says that an undeclared bare name is always a load error. Clarify that only a standalone where: "name" atom is rejected.
Proposed wording
-An undeclared *bare* name is a load error, and a mask dim outside `foreach` is one too (§5.2).
+An undeclared *standalone* bare name is a load error; an undeclared RHS name
+in `name OP value` is read as a string coordinate. A mask dim outside
+`foreach` is one too (§5.2).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| everything out. An undeclared *bare* name is a load error, and a mask dim outside `foreach` is | |
| one too (§5.2). | |
| everything out. An undeclared *standalone* bare name is a load error; an undeclared RHS name | |
| in `name OP value` is read as a string coordinate. A mask dim outside | |
| `foreach` is one too (§5.2). |
🤖 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 `@docs/SPEC.md` around lines 387 - 388, Clarify the load-error rule in the
surrounding documentation so it applies only to an undeclared standalone where
atom of the form “where: "name"”, while preserving the preceding rule that
permits undeclared names used as RHS string coordinates. Update the sentence
near the bare-name and mask-dimension rules without changing unrelated
semantics.
Comment on lines
+405
to
+427
| `array` is any node of the right dim set, so `shift` re-indexes a **parameter** | ||
| as readily as a variable: `shift(dt, over=t, by=1, edge=0)` is the previous | ||
| snapshot's duration, without shipping a pre-shifted copy of a table the model | ||
| already has. | ||
|
|
||
| Four rules govern `edge=`, and all four are law 8 in this position: | ||
|
|
||
| - **Bare** — the vacated coordinate is absent in exactly §6's sense, so an | ||
| acyclic recurrence has no row at its first coordinate rather than a row | ||
| asserting the quantity starts at zero. An initial condition is then something | ||
| the model states, under a complementary `where`. | ||
| - **Numeric** — asks for a value back, and it is a number rather than a flag | ||
| because the identity is positional: `0` for a sum, `1` for a product. The | ||
| library cannot see which position it is in and the model can. | ||
| - **Over a variable, the only representable numeric edge is `0`** — a vacated | ||
| slot there contributes no term at all, and a nonzero one would be a constant | ||
| standing where a term was. | ||
| - **A bare `shift` over a variable-free expression is a load error.** A | ||
| parameter's missing row is a zero coefficient (§6), so there is no absence for | ||
| the vacated slot to carry, and inventing one silently turns | ||
| `x <= shift(dt, over=t, by=1)` into `x <= 0`. The error names the three things | ||
| it could have meant: `edge=0`, a `where` masking the coordinate out, or | ||
| `edge=wrap`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Qualify the parameter-shift rule.
The section first says that shift accepts any node, then rejects a bare shift over a variable-free expression. State the exception in the first paragraph. Otherwise, shift(dt, over=t, by=1) appears valid but fails at load time.
Proposed wording
-`array` is any node of the right dim set, so `shift` re-indexes a **parameter**
+With an explicit `edge=` argument, `array` may be any node of the right dim set,
+so `shift` re-indexes a **parameter**📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `array` is any node of the right dim set, so `shift` re-indexes a **parameter** | |
| as readily as a variable: `shift(dt, over=t, by=1, edge=0)` is the previous | |
| snapshot's duration, without shipping a pre-shifted copy of a table the model | |
| already has. | |
| Four rules govern `edge=`, and all four are law 8 in this position: | |
| - **Bare** — the vacated coordinate is absent in exactly §6's sense, so an | |
| acyclic recurrence has no row at its first coordinate rather than a row | |
| asserting the quantity starts at zero. An initial condition is then something | |
| the model states, under a complementary `where`. | |
| - **Numeric** — asks for a value back, and it is a number rather than a flag | |
| because the identity is positional: `0` for a sum, `1` for a product. The | |
| library cannot see which position it is in and the model can. | |
| - **Over a variable, the only representable numeric edge is `0`** — a vacated | |
| slot there contributes no term at all, and a nonzero one would be a constant | |
| standing where a term was. | |
| - **A bare `shift` over a variable-free expression is a load error.** A | |
| parameter's missing row is a zero coefficient (§6), so there is no absence for | |
| the vacated slot to carry, and inventing one silently turns | |
| `x <= shift(dt, over=t, by=1)` into `x <= 0`. The error names the three things | |
| it could have meant: `edge=0`, a `where` masking the coordinate out, or | |
| `edge=wrap`. | |
| With an explicit `edge=` argument, `array` may be any node of the right dim set, | |
| so `shift` re-indexes a **parameter** | |
| as readily as a variable: `shift(dt, over=t, by=1, edge=0)` is the previous | |
| snapshot's duration, without shipping a pre-shifted copy of a table the model | |
| already has. | |
| Four rules govern `edge=`, and all four are law 8 in this position: | |
| - **Bare** — the vacated coordinate is absent in exactly §6's sense, so an | |
| acyclic recurrence has no row at its first coordinate rather than a row | |
| asserting the quantity starts at zero. An initial condition is then something | |
| the model states, under a complementary `where`. | |
| - **Numeric** — asks for a value back, and it is a number rather than a flag | |
| because the identity is positional: `0` for a sum, `1` for a product. The | |
| library cannot see which position it is in and the model can. | |
| - **Over a variable, the only representable numeric edge is `0`** — a vacated | |
| slot there contributes no term at all, and a nonzero one would be a constant | |
| standing where a term was. | |
| - **A bare `shift` over a variable-free expression is a load error.** A | |
| parameter's missing row is a zero coefficient (§6), so there is no absence for | |
| the vacated slot to carry, and inventing one silently turns | |
| `x <= shift(dt, over=t, by=1)` into `x <= 0`. The error names the three things | |
| it could have meant: `edge=0`, a `where` masking the coordinate out, or | |
| `edge=wrap`. |
🤖 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 `@docs/SPEC.md` around lines 405 - 427, Update the opening paragraph describing
shift’s accepted nodes to explicitly qualify that variable-free expressions are
not valid with a bare edge. State that such parameter shifts require an explicit
edge value, masking via where, or edge=wrap, while preserving the subsequent
four edge rules.
Comment on lines
+440
to
+446
| 1. a key in `sources` — a table carrying a column of that name, or a parquet | ||
| path; first occurrence of each value is its position | ||
| 2. `coords=` — anything `pd.Index()` accepts, or a table carrying the label | ||
| column plus one column per declared coordinate (§2) | ||
| 3. `values:` in the YAML | ||
| 4. *streaming lane only* — derived from the parameter tables that carry the | ||
| dim, as **sorted** distinct values | ||
|
|
||
| Step 4 is unavailable to a dimension declaring `coords` — it reads index | ||
| columns only, so it cannot supply a coordinate. Otherwise step 4 exists because | ||
| a dim some parameter already spans needs no second declaration, but it costs the *declared order*, which `shift` reads | ||
| positionally — so pass an explicit index whenever order matters. The linopy | ||
| lane has no step 4: a dimension with neither `coords=` nor `values:` raises | ||
| there. A dim that no source names and no parameter carries raises on both. | ||
|
|
||
| **Accepted per parameter** (declared `dims: [d1, d2]`), streaming lane: a | ||
| parquet path; any table exposing the Arrow PyCapsule protocol with columns | ||
| `d1, d2, value`; `int`/`float` for a 0-D parameter. `pd.Series` and | ||
| `xr.DataArray` keep their dims in an *index* rather than in columns, so they | ||
| are unwrapped first — but only if that library is already imported, never by | ||
| importing it. | ||
|
|
||
| Compat lane (`data=`): `int`/`float` as a scalar that broadcasts freely; `dict` | ||
| and `pd.Series` for 1-D (keys / index values become coordinates); | ||
| `pd.DataFrame` for 2-D (index name → `d1`, column name → `d2`); `xr.DataArray` | ||
| directly, with dim names a subset of the declared dims. `np.ndarray` and `list` | ||
| have no named axes, so only 0-D or 1-D matching one declared dim is accepted — | ||
| anything else is refused with a message asking for a named object. | ||
|
|
||
| Index names are optional but **binding**: an unnamed index binds positionally | ||
| to the declared `dims`, a named one binds by name in any order, and a name | ||
| outside the declared dims raises rather than being overwritten. | ||
| 4. derived from the parameter tables that carry the dim, as **sorted** distinct | ||
| values |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Identify sources and coords= as loader inputs.
The specification defines a closed YAML schema with eight top-level keys. This section introduces sources and coords= without stating that they are loader/API arguments. Readers may add them to YAML and receive a closed-schema error.
Proposed wording
-1. a key in `sources` — a table carrying a column of that name, or a parquet
+1. a key in the loader's `sources` mapping — a table carrying a column of that name, or a parquet
path; first occurrence of each value is its position
-2. `coords=` — anything `pd.Index()` accepts, or a table carrying the label
+2. the loader's `coords=` argument — anything `pd.Index()` accepts, or a table carrying the label📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 1. a key in `sources` — a table carrying a column of that name, or a parquet | |
| path; first occurrence of each value is its position | |
| 2. `coords=` — anything `pd.Index()` accepts, or a table carrying the label | |
| column plus one column per declared coordinate (§2) | |
| 3. `values:` in the YAML | |
| 4. *streaming lane only* — derived from the parameter tables that carry the | |
| dim, as **sorted** distinct values | |
| Step 4 is unavailable to a dimension declaring `coords` — it reads index | |
| columns only, so it cannot supply a coordinate. Otherwise step 4 exists because | |
| a dim some parameter already spans needs no second declaration, but it costs the *declared order*, which `shift` reads | |
| positionally — so pass an explicit index whenever order matters. The linopy | |
| lane has no step 4: a dimension with neither `coords=` nor `values:` raises | |
| there. A dim that no source names and no parameter carries raises on both. | |
| **Accepted per parameter** (declared `dims: [d1, d2]`), streaming lane: a | |
| parquet path; any table exposing the Arrow PyCapsule protocol with columns | |
| `d1, d2, value`; `int`/`float` for a 0-D parameter. `pd.Series` and | |
| `xr.DataArray` keep their dims in an *index* rather than in columns, so they | |
| are unwrapped first — but only if that library is already imported, never by | |
| importing it. | |
| Compat lane (`data=`): `int`/`float` as a scalar that broadcasts freely; `dict` | |
| and `pd.Series` for 1-D (keys / index values become coordinates); | |
| `pd.DataFrame` for 2-D (index name → `d1`, column name → `d2`); `xr.DataArray` | |
| directly, with dim names a subset of the declared dims. `np.ndarray` and `list` | |
| have no named axes, so only 0-D or 1-D matching one declared dim is accepted — | |
| anything else is refused with a message asking for a named object. | |
| Index names are optional but **binding**: an unnamed index binds positionally | |
| to the declared `dims`, a named one binds by name in any order, and a name | |
| outside the declared dims raises rather than being overwritten. | |
| 4. derived from the parameter tables that carry the dim, as **sorted** distinct | |
| values | |
| 1. a key in the loader's `sources` mapping — a table carrying a column of that name, or a parquet | |
| path; first occurrence of each value is its position | |
| 2. the loader's `coords=` argument — anything `pd.Index()` accepts, or a table carrying the label | |
| column plus one column per declared coordinate (§2) | |
| 3. `values:` in the YAML | |
| 4. derived from the parameter tables that carry the dim, as **sorted** distinct | |
| values |
🤖 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 `@docs/SPEC.md` around lines 440 - 446, Clarify in the specification’s
precedence list that `sources` and `coords=` are loader/API inputs rather than
YAML keys. Update the entries describing these two inputs while preserving the
existing precedence and value-resolution behavior, so readers do not add them to
the closed YAML schema.
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 contradiction this fixes
The docs said two incompatible things about quadratic, and I made it worse in #378 by adding sequencing without removing the refusal:
ceiling.mdlisted "degree 1 (affine)" as one of three admissibility rulesA reader could not tell which was the position, because all three were on the page.
The position, stated once
The ceiling is
relational ∩ local. That protects streaming, it is solver-independent, and it is not moving.Degree is not part of it. Nothing about
variable × variableis non-relational or non-local — a coordinate-aligned product is a pointwise self-join. Degree 1 is where the language is, not where it must stay. What actually gates quadratic is what a sink can ingest, and the same is true of SOS, indicator and semi-continuous.So
affine ∩ relational ∩ localbecomesrelational ∩ local— inceiling.md,ROADMAP.md,index.mdandREADME.md, which all carried the old phrase.What moves out of "non-primitives"
Into Track 3 — capabilities, and the degree line, ordered by effort behind the capability model:
kSemiContinuous, linopy has the oracleQuadratic's section now opens "Quadratic is planned, not refused" and says plainly that performance is not the question.
piecewise: {convex: true}stays the preferred spelling for convex 1-D curves even after it lands, because it keeps LP duals and MILP compatibility — that is a recommendation, not a refusal.Cumulative sums over a variable were already moved from banned to priced (#380). What is left in the refusals table is only what is actually refused: data prep, domain helpers, a variable divisor, structure inside one plan, and a Python modeling API.
Two stale references found while editing
ceiling.mdcited "rule 6" twice for the no-Python-modeling-API rule. ARCHITECTURE's hard rules run 0–5, and it is rule 5.Track 2canchor pointed at a heading that no longer exists.Renumbering the tracks (sink capabilities 4→3, memory 5→4) moved four inbound anchors; the link text moved with them rather than naming a track that no longer has that number.
mkdocs build --strictcatches exactly this and now reports zero warnings.Also in this PR (first commit)
SPEC.mdARCHITECTURE.mdROADMAP.mdapi.mdSPEC §10 moved to
docs/api.md— the page's header says it documents "what a YAML file may contain and what it means", and how to callsolve()is not that. §10 is a three-line pointer, so §11's number and every inbound link are untouched.Hazard worth knowing: that move silently dropped six tests.
tests/test_doc_examples.pyhas aTRACKEDlist of docs whose code blocks are compiled; the suite went 709 → 703 and nothing failed, it just stopped checking.api.mdis inTRACKEDnow.ARCHITECTURE also states the label contract once instead of three times, with room to add that appending rows is safe — the fact decomposition depends on.
709 passed,
mkdocs build --strictzero warnings.🤖 Generated with Claude Code
Summary by CodeRabbit