Fix coordinate misalignment in expression merge#550
Conversation
…ing logic - Mark defensive code paths with pragma: no cover (unreachable in practice)
|
Im not entirely sure if this affects performance, but its at least worth raising i thought |
…ion-based approach. For each non-helper dimension, it computes the union of coordinate values across all datasets. If any mismatch is found, all datasets are reindexed to the union coordinates with FILL_VALUE defaults. This handles both the reordered-coords case and the overlapping-subset case. test/test_linear_expression.py: Added test_merge_with_overlapping_coords that creates variables with ["alice", "bob"] and ["bob", "charlie"], merges them, and verifies correct alignment — bob gets both terms, alice and charlie get only their respective term with fill values for the missing one.
…es) and then patching up mismatches with reindexing, _check_coords_match now checks that actual coordinate values and order are identical. When they're not, override is False and xarray's join='outer' handles alignment correctly. The entire reindex block is gone.
|
this is a very sensible API change! let's pull that in. do you see anything missing here? |
|
I would like to reconsider if there is a vlid use case for this, or if its truly only a bug. |
|
@FabianHofmann
So the behaviour change is for such cases: import pandas as pd
import numpy as np
import linopy
m = linopy.Model()
hours = pd.date_range("2025-01-01", periods=8760, freq="h", name="time")
gen = m.add_variables(coords=[hours], name="gen")
# Suppose demand comes from an external source, sorted differently
demand = pd.Series(
data=np.linspace(0, 100, 8760),
index=hours.sort_values(ascending=False), # reversed!
name="demand",
)
# Before the PR fix, this silently misaligns:
expr = gen - demand
print(expr)If the indexes were not both containing equal keys, nothing changes (shift, subset, etc were always aligned correctly) |
|
yes, I agree. did you test if this now also applies to |
Im not sure. Should i verify the call graph? |
|
That would be awesome, but if not also fine. I guess I have to sit down next week and figure out a consistent convention for all linopy operations anyway |
|
@FabianHofmann I'll see if i find the time. Unfortunately i will be quite busy next week, except friday. So if you need someone to brainstorm with and you can wait until friday tell me ;) |
|
@FabianHofmann This was not fixed with #572 |
|
Should be addressed in #591 |
Documents 5 categories of legacy issues with paired legacy/v1 tests: 1. Positional alignment (#586, #550): same-shape operands with different labels silently paired by position, producing wrong results 2. Subset constant associativity (#572): left-join drops coordinates, making (a+c)+b != a+(c+b) 3. User NaN swallowed (#620): NaN in user data silently filled with inconsistent neutral elements (0 for add/mul, 1 for div) 4. Variable vs Expression inconsistency (#569, #571): x*c and (1*x)*c previously gave different results 5. Absent slot propagation (#620): legacy can't distinguish absent variables from zero, fillna() is a no-op Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
`_align_constant` branches on `options["semantics"]`: v1 uses exact alignment via `xr.align(join="exact")`; legacy keeps the size-aware positional/left-join behaviour and emits `LinopySemanticsWarning` when v1 would diverge. `_add_constant`/`_apply_constant_op` raise on a NaN in a user-supplied constant under v1, warn under legacy. `Variable.__mul__(DataArray)` now routes through `to_linexpr() * other` so the LinearExpression checks fire; the scalar fast-path is preserved (a NaN scalar diverts to the expression path so v1 raises). Marks the bug-class test groups `TestCoordinateAlignment` (#708/#586/ #550), `TestConstraintCoordinateAlignment`, `TestNaNMasking`, `test_auto_mask_constraint_model`, and four piecewise NaN-padding tests as `@pytest.mark.legacy` — they assert the very behaviour v1 forbids. v1 coverage of those bug classes accretes via later slices. `test/test_legacy_violations.py` (new) adds 22 paired tests covering §5/§8/§9 plus the PyPSA #1683 `0*inf=NaN` case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Superseded by #758, which fixes this bug for both the constant and expression- Note Posted via Claude Code on the maintainer's instruction. |
* fix(alignment): align reordered shared-dim coords by label in merge (#550) §8 aligns by label, not position, so the same labels in a different order are the same coordinate. The constant path already reindexed a pure reorder, but the expression-merge path raised under v1 and silently misaligned under legacy. merge() now conforms shared user dims to the first operand's order before the §8/§11 checks, so a reorder reindexes (correct under both semantics) while a differing label set still raises. Aux coords ride along the reindex, so §11 conflicts are preserved. Spec: convention.md §8 now states order-independence explicitly and is retitled "Shared dimensions must carry the same labels" (was framed as xarray's order-sensitive `exact`). Supersedes #550. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(merge): fold reorder-conform and §8 mismatch detection into one pass The reorder fix added a second walk over the shared user dims (one to reindex reordered coords, one to detect a genuine mismatch), duplicating the per-dim .equals() work on every join=None merge — the hot path during model building. conform_merge_dims does both in a single pass and replaces merge_shared_user_coord_mismatch + the separate conform helper. Behaviour is unchanged (full suite 6476 passed under both semantics). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(merge): pin reorder behaviour on multi-operand, quadratic, and MultiIndex paths Regression guards for the §8 by-label alignment beyond the 2-operand case: multi-operand merge([a,b,c]) pairs by label, quadratic merge aligns reordered dims, and a reordered stacked MultiIndex raises (xarray cannot reindex it by tuple — left to §11; tied to the #744 MultiIndex-storage decision). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(merge): align reordered stacked MultiIndex by tuple (resolve §8/§11 gap) A reordered full MultiIndex is "the same coordinate spelled differently" and per §8/§11 should align — but reindex cannot reorder a stacked MI by tuple, so it previously fell through to a confusing §11 aux-coord raise. conform_merge_dims now permutes via positional isel using get_indexer, which works uniformly for a plain index and a MultiIndex's tuples (and get_indexer doubles as the same-set test, replacing the set() comparison — cheaper). A genuinely different label set still raises the §8 mismatch. convention.md §11 now states order-independence for the full-MI case explicitly. Only the MultiIndex case was affected; a plain dim with aux coords already reordered correctly (the aux coord rides along the permute). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(merge): raise dim mismatch before the aux-coord check A shared-dim label mismatch is the root cause, so report it as a dim conflict rather than letting the §11 aux check fire first — a different stacked MultiIndex otherwise surfaced as its level coords conflicting (the wrong message). Aux conflicts still raise once the dims agree. Adds a routing test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(merge): assert via public .indexes, not .coeffs.coords The reorder tests reached through the internal term storage (.coeffs.coords) for coordinates that the expression exposes directly via .indexes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(merge): make reorder-align v1-only; legacy keeps positional + warns Per the transitioning contract, legacy must not change: reordering coords on a shared dim was always positional in expression merges (the constant path, by contrast, has always aligned labelled operands by label — that asymmetry is genuine legacy and is what v1 unifies). conform_merge_dims now permutes only under v1; under legacy it leaves the operands positional and the caller warns with a reorder-specific message (v1 would align by label, a different result). Tests: the align cases are now @pytest.mark.v1; added legacy guards for the positional result and the full warning text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Correction to the previous unmark commit. test_var_plus_var_reordered_ labels_align and test_quadratic_merge_reordered_aligns only asserted .indexes, which is identical under both semantics — so they looked like equal-behaviour tests, but assert_linequal/assert_quadequal show the *variable pairing* genuinely diverges: v1 pairs by label, legacy pairs positionally (preserving master's #550 behaviour). Re-mark both v1, strengthen them to assert the by-label pairing (so they actually verify the #550 fix, not just the index order), and add legacy twins pinning the positional pairing. Found by re-verifying the unmark set with linopy.testing's strict structural comparison instead of trusting each test's own assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes silent data corruption when adding expressions whose coordinates have the same values but in different order (e.g.
['costs', 'Penalty']vs['Penalty', 'costs']). Previously,merge()used positional concatenation viajoin='override', ignoring labels entirely.What changed
When
merge()detects that two datasets share the same coordinate values in a different order, it now reindexes to the first dataset's order before concatenation. This only triggers when the coordinate value sets are identical — different subsets or different sizes are unaffected.What is NOT affected
v.loc[:9] + v.loc[10:]— different value sets, override preservedBug reproduction script
Benchmark script (
dev-scripts/benchmark_merge_alignment.py)Checklist
doc/release_notes.rstis included