refactor(common): split DataArray conversion into a 3-rung strictness ladder#737
Conversation
… ladder Replace the as_dataarray + _as_dataarray_lax pair (and the enforce_level_coverage flag) with three public entry points, each including the previous one: - as_dataarray: convert only (the former _as_dataarray_lax). Used by __matmul__, where dims missing from the constant must not be broadcast in (they would be contracted away as common dims). - broadcast_to_coords: convert + broadcast against coords (the former broadcasting as_dataarray). Used by expression arithmetic. - align_to_coords: convert + broadcast + enforce the coords contract. Used by add_variables / add_constraints (unchanged signature). The broadcasting mechanics live in one shared private core (_broadcast_core) that reports MultiIndex-level projections instead of applying policy. The entry points decide what a partial projection or coverage gap means: broadcast_to_coords warns (arithmetic convention), align_to_coords raises (coords contract). This removes the enforce_level_coverage flag and keeps validation concerns out of the broadcasting layer. No behavior changes; all call sites keep their semantics. New tests pin the ladder contrasts and the matmul dim-contraction rules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@FabianHofmann For me the whole broadcasting and alignment in linopy is at least as difficult to undnerstand as the arithmetics. And arithmetics also rely on this broacasting. SO i think getting this right, with clear methods that have distinct, understandable roles is a really important step. Thats why im putting this much effort into it. And thanks for your work aout Multiindex. Im not really that good in that area |
Private-twin convention: _broadcast_to_coords is the raw implementation of broadcast_to_coords (returns projection events instead of applying policy), shared with align_to_coords. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… rung The constraint lhs/rhs setters call as_expression(value, model, coords=self.coords, dims=self.coord_dims); forwarding those kwargs to the convert-only as_dataarray dropped the broadcasting these setters relied on (e.g. a MultiIndex-level-indexed rhs failed with an xarray AlignmentError instead of being projected onto the stacked dim). Use broadcast_to_coords instead. The other as_expression callers pass only dims (no coords), for which both rungs behave identically. Adds regression tests for the rhs setter: missing-dim broadcast and MultiIndex-level projection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@FBumann thanks for taking another look, I read the code and the tests. it all makes sense. it is a complicated thing and the time spent here is definitely worth it. I am wondering whether we are already at the optimum. still thinking about the arithmetics the generalization we have there is the I would see that the final API could be equivalent to most of the calls of as_dataarray + reindex/broadcase calls that we have could be replaced by one DetailsConvergence targetas_dataarray(arr, coords, dims) # convert only — matmul
align_to_coords(arr, coords, *, join="defer", # convert + broadcast + reconcile [+ validate]
enforce_dims=False, fill_value=NA,
dims=None, label=None)Two public functions instead of three. What to change in #7371. Don't ship
2. Put for dim, coord_values in expected.items():
...
if actual_idx.equals(expected_idx):
continue
same_set = len(actual_idx) == len(expected_idx) and set(actual_idx) == set(expected_idx)
if join == "defer":
if same_set: # today's behavior verbatim
arr = arr.reindex({dim: expected_idx})
elif join == "left":
arr = arr.reindex({dim: expected_idx}, fill_value=fill_value) # NEW
elif join == "override":
if len(actual_idx) == len(expected_idx):
arr = arr.assign_coords({dim: expected_idx})
# join == "exact": leave values; the contract check raises (see #3)
3. Make the policy a parameter keyed off enforce_dims: Literal[False, "warn", "raise"] = False
The 4. Exclude 5. Migrate call sites in #737 (atomically).
6. Demonstrate
This makes the What stays out of scope (and why — confirmed by the exploration)
Net effect on #737Same diff size, same zero-behavior-change guarantee, same matmul/flag fixes — but the public surface lands as Two decisions for you
Want me to check out |
|
@FabianHofmann I think the important thing is to get the mental model right and to know what is done where in the codebase. This will be achieved with this PR i think. After that, refactoring to the proposed design seems much simpler. However, Im not sure of going from 3 back to 2 methods is desireable. Its 3 very distinct methods. A join parameter in general is a good idea I think, but if we add an ambigous option like "defer", which is not unknown for a join, this creates new ambiguity i think. My proposal would be: Lets merge this PR, then look if we need a refactoring back to 2 mehods and a join parameter after all. |
|
@FabianHofmann I thought about the The pattern:
|
| operation | reference = another object | reference = explicit coords |
|---|---|---|
| convert | — | as_dataarray(arr, coords, dims) |
| broadcast — make dims agree | Variable.broadcast_like(other) ✓ |
broadcast_to_coords(arr, coords) |
| reindex — make entries agree, fill gaps | reindex_like(other) on expressions / constraints ✓ |
reindex_to_coords(arr, coords, fill_value=…) ← NEW (your join="left") |
| exact check — entries must agree, else raise | align(a, b, join="exact") ✓ |
strict_broadcast_to_coords(arr, coords, label=…) (today's align_to_coords) |
| inner / outer join — both sides change | align(a, b, join=…) ✓, operator join= ✓ |
impossible |
Three things the matrix shows:
1. The family completes what linopy already half-has. Each _to_coords function is the coords-flavored sibling of an existing method. Nothing exotic.
2. Why there's no join= parameter in the right column. A join that changes both sides (inner / outer) cannot be completed there: coords is frozen, and the expression it came from isn't in the function's hands — it can't be grown to match. The only place holding both sides is the operator (.add / .mul / .le (..., join=) → _align_constant), which is where inner / outer already live. What remains expressible against a frozen reference is exactly: check (exact) or conform (left) — two functions, not a parameter.
3. A purity note. xr.broadcast / broadcast_like quietly outer-align conflicting shared-dim entries before broadcasting (they can — they hold both objects). broadcast_to_coords deliberately doesn't: a half-completed join is worse than none, so entry conflicts pass through untouched to the operator's join=. This matches v1 §9, where broadcasting is dims-only and entry conflicts are §8's business.
The rename: align_to_coords → strict_broadcast_to_coords
The strict rung never aligns anything — it checks and raises. "Align" is also exactly the word that invited the join= idea: aligns take joins, broadcasts don't. Naming it as what it is — the same broadcast with a strict failure mode — makes the no-join design self-enforcing, and puts the required label argument on the one function that needs it.
Follow ups
The join="left" capability — follow-up PR as reindex_to_coords, migrating the two broadcast + reindex_like call sites (to_linexpr coefficient, to_constraint rhs) so it ships with real users. #737 stays purely structural.
Refined with Claude Code.
The function never aligns anything — it broadcasts and raises on any mismatch it cannot resolve by broadcasting alone. "Align" is also the word that invites join= proposals (aligns take joins, broadcasts do not), so the name now states what it is: the same broadcast as broadcast_to_coords with a strict failure mode (zip(strict=True) semantics). Error messages keep the "could not be aligned to coords" wording so tests in the base branch (#732) stay untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
I prototyped Reason 1: Reason2: |
- Document the one non-obvious policy in strict_broadcast_to_coords: partial-level broadcasts are silent (bounds-broadcast feature), unlike the warning on the broadcast rung. - Unify the first parameter name across the ladder (value -> arr). - Un-invert the warning-policy loop in broadcast_to_coords. - Rename the test whose name forced an awkward signature wrap to a behavior-oriented name (test_extra_dims_pass_broadcast_rung_fail_strict_rung). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Parameter entries carry descriptions only — types live in the function signatures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
sounds good. we are moving in the right direction. strict_broadcast and broadcast are differing in the MI checks. I would argue the latter should go to both. Second, and that is on me, the partial level coverage should be supported in future (I have to check compliance with v1 conventions again quickly). that said, I still think the two should live in one function |
I would leave the MI stuff up to you. I never use MI and therefore dont know whats needed. If it can be the same/extracted, thats a win i think! And if the MI stuff is equal, both methods could be one. I'd suggest the signature: broadcast_to_coords(arr, coords=None, dims=None, *, strict=True, label=None, **kwargs)
That said, we can only unify the methods if we actually unify what strict means:
So I'd wait for your v1 check — once strict means one thing, the merge is mechanical. EDIT |
|
great, let's go |
|
@FabianHofmann You where to fast. I edited it a bit. Did you check v1? |
|
@FBumann let me reconcile my thoughts on the partial coverage. you do the signature change, use MI handling of today's strict version and I make some research again how MI is supported in upcoming xarray versions (which could change the picture). so don't mind the MI handling (perhaps don't deleted the checks as well), merge this one as soon as you feel ready and I take another look at MI in #732 |
|
Note Claude Code summary — structuring the open MI question so it's a one-table decision. The
So the unified A — v1 allows partial-level projection (stays a feature)
→ the #732 partial-level warning is removed; #717 needs an amendment legitimizing the projection (§9 extended to MI levels). B — v1 forbids implicit projection (#717 as written, §8/§11)
→ per-period bounds (PyPSA multi-investment) become deprecated usage with a migration path before v1; #717 stays as written. Bottom line: pick A or B; the merge into one function is mechanical after that. A = amend the convention. B = deprecate what #732 just shipped. @FabianHofmann this really helped me |
|
we should learn from xarry community which struggled a lot with MI's it seems - let's pick B! |
|
Note Claude Code reference note — the MultiIndex representation that underlies every MI issue in this stack. Useful background for the #732 MI follow-up. Click to expandA pandas MultiIndex has two xarray representations, and the friction between them is the root of all the MI complexity here: Stacked: 1 dim + level coords (what linopy / PyPSA use)One dimension; each level is a non-dimension (auxiliary) coordinate attached to it. Can represent sparse indexes (only the combinations that exist). Unstacked: 2 dims (what
|
| Issue | Cause |
|---|---|
| Partial-level broadcast (per-period bounds) | input has period as a dim; target has period as a level coord. Same name, different role → the projection translates between the two representations |
| v1 §11 applies (aux-coord conflict) | level coords are auxiliary coords — a level-indexed operand is exactly the §11 case |
| pydata/xarray#11368 (reindex fails) | reindexing the stacked dim must also rewrite the level coords; raw indexers don't know they exist |
expand_dims workaround in #732 |
expanding a missing MI dim must create dim + all level coords, not just the dim |
| Coverage gaps | only exist when conforming a sparse stacked index to a full one — the unstacked form can't even express sparsity except as NaN |
So for the #732 MI follow-up: the question "how is MI supported in upcoming xarray versions" is concretely "does xarray's indexer API learn to handle level coords" (#11368) — the stacked form itself is stable; it's every operation that crosses between the two forms that needs hand-holding.
yes, that is all correct but no blocker right? atm we support partial level coverage ie. allowing levels as indexes, but this will change in future (warn now, raise later) |
Yes, i just wanted to dump some context into this PR. Not blocking |
…strict=...) Per review discussion: one public function instead of two, with strict as a keyword flag. - strict=True (default): any mismatch with coords raises, naming label in the error — the former strict_broadcast_to_coords. - strict=False: mismatches pass through for downstream xarray alignment — the former loose broadcast_to_coords, used by arithmetic. Strict is the default so that forgetting the flag adds safety rather than silently dropping validation. MI handling preserved exactly per mode (strict: silent partial / raise on gap; non-strict: EvolvingAPIWarning) — the scenario-B deprecation warnings land separately in #732. Call sites: model.py bounds/mask drop the long name (strict is default); arithmetic and as_expression pass strict=False explicitly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restores the contract align_to_coords always had: strict-mode errors must
name their subject ("lower bound could not be aligned..." rather than
"Value could not be aligned..."). Enforced both statically (overloads:
strict=True requires label: str, strict=False forbids it) and at runtime
(TypeError).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ario B) Per the #737 review discussion and Fabian's decision: implicit level projection is deprecated and will raise under the v1 convention, so the EvolvingAPIWarning now fires in both modes of broadcast_to_coords — the MI check is the same for every use case: - input missing a whole level: warn (strict and non-strict) - coverage gap (level combinations without a value): warn (non-strict) / raise (strict — no downstream layer to defer the NaN to) Warning emission lives in one helper, _warn_implicit_projections, with a TODO(#738) to migrate to LinopySemanticsWarning once #717 lands. Also clarifies the MultiIndex terminology everywhere: an MI dim has *levels* and *level combinations* (one tuple per position). Docstrings carry the glossary, the coverage-gap error names the missing combinations explicitly, and "entry" is gone from messages. User-facing: add_variables / add_constraints with per-period-style bounds now emit the deprecation warning (PyPSA multi-investment). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@FabianHofmann Ready |
Stacked on #732. Resolves the open questions from the #732 review thread by re-layering
linopy.common's DataArray conversion. The design was developed in this PR's comment thread.What
Two public entry points — replacing the
as_dataarray-with-flag + private_as_dataarray_lax+align_to_coordstrio:strictdecides what happens to anything broadcasting alone cannot resolve (extra dims, disagreeing coord values, MI coverage gaps):strict=True(default): raise, naminglabelin the error (labelis required in this mode — enforced via overloads and at runtime). Forgetting the flag adds safety instead of silently dropping validation.strict=False: pass through for downstream xarray alignment (the operator'sjoin=owns reconciliation).Backed by one private mechanics function (
_broadcast_to_coords) that reports MultiIndex projections (_LevelProjection) instead of deciding what they mean; the public function applies policy per mode.Caller profiles
__matmul__(2 sites) — broadcasting would contracta=(time,name) @ b=(name,location)to(location)instead of(time,location)as_dataarray(other, coords=…, dims=…)add_variables/add_constraintsbounds + maskbroadcast_to_coords(lower, coords, label="lower bound")to_linexpr,as_expression(constraintlhs/rhssetters)broadcast_to_coords(other, coords=…, strict=False)MultiIndex policy (scenario B — decided in this thread)
Terminology: a stacked MultiIndex dim has levels (its component index names, e.g.
period/timestep) and level combinations (its elements — one tuple per position, e.g.(2030, 't1')).Implicit level projections are deprecated everywhere and will raise under the v1 convention — the MI check is the same in both modes:
strict=False(arithmetic)strict=True(bounds/mask)EvolvingAPIWarningEvolvingAPIWarningEvolvingAPIWarningValueError— the error lists the missing combinationsThe warning channel carries a
TODO(#738): migrateEvolvingAPIWarning→LinopySemanticsWarningonce #717 lands.This removes the
enforce_level_coverageflag and the cross-module use of a private helper — the two things the #732 thread flagged.Behavior changes
One, deliberate (scenario B):
add_variables/add_constraintswith inputs indexed by a subset of a MultiIndex's levels (e.g. PyPSA's per-period bounds) now emit theEvolvingAPIWarningdeprecation warning. Everything else keeps the semantics it has on #732. (Also caught in review and fixed: the constraintlhs/rhssetters go throughas_expression, which now usesstrict=False— with regression tests.)Tests
as_dataarraydoesn't expand /broadcast_to_coords(strict=False)passes mismatches through /strict=Truerejects them with labeled errors, stays silent on partial-level bounds.test_matmul_contracts_only_shared_dims: pins(dim_0,dim_1) @ (dim_1,location) → (dim_0,location).rhssetter regressions: missing-dim broadcast + MultiIndex-level projection.Out of scope (agreed in the thread)
reindexonto a stacked MI is broken upstream).reindex_to_coords— deferred until a coords-only caller exists; blocked on Cannot reindex onto a stacked MultiIndex via indexers — only reindex_like works pydata/xarray#11368.EvolvingAPIWarning→LinopySemanticsWarning) — Unify deprecation warning channels before v1: EvolvingAPIWarning vs LinopySemanticsWarning #738, blocked on feat: v1 semantic convention #717.🤖 Generated with Claude Code