Skip to content

test: pin alignment contract for variable/expression/constraint bugs - #715

Closed
MaykThewessen wants to merge 1 commit into
PyPSA:masterfrom
MaykThewessen:test/alignment-contract-tests
Closed

test: pin alignment contract for variable/expression/constraint bugs#715
MaykThewessen wants to merge 1 commit into
PyPSA:masterfrom
MaykThewessen:test/alignment-contract-tests

Conversation

@MaykThewessen

Copy link
Copy Markdown
Contributor

Summary

Adds test/test_alignment_contract.py with six xfail(strict=True) regression
tests that pin the intended coordinate-alignment contract reported across the
open alignment-bug cluster (#586, #706, #707, #708, #709).

The bugs all describe the same root issue: linopy's operators and
add_variables handle coordinate alignment inconsistently — some by label,
some positionally, some silently dropping dims. The discussion on each issue
keeps re-litigating what the contract should be. A failing test pins the
answer: every operator aligns by label, add_variables preserves coords
order and shape regardless of bound type.

Issue → test mapping

IssueTestContract pinned
#707test_subtract_then_le_matches_direct_lex - a <= 0 and x <= a build the same constraint
#708test_add_aligns_by_labelx + a aligns by label, not by position
#708test_subtract_aligns_by_labelx - a aligns by label
#708test_divide_aligns_by_labelx / a aligns by label and x / a == x * (1 / a)
#706test_add_variables_dataarray_bounds_preserve_coords_orderadd_variables dim order follows coords for DataArray bounds with missing dims
#709test_add_variables_pandas_bounds_broadcast_missing_dimadd_variables broadcasts pandas bounds to full coords shape

#586 is intentionally not included as its own test. Its original repro
(m.add_constraints(x >= series) with reordered labels) no longer reproduces
on master — the add_constraints path now label-aligns correctly. #707's
description already notes this: "#586's own snippet … takes the label-aligned
path and no longer reproduces, but the bug is alive in arithmetic." The
surviving arithmetic piece is covered by the #707 / #708 tests above, so
#586 is effectively merged into that coverage.

Why xfail strict=True

Each test asserts the expected behaviour (the bug fixed), not the current
buggy output. As long as the bug reproduces the test xfails; the moment the
fix lands the test starts passing, strict=True turns the XPASS into a CI
failure, and the maintainer is forced to remove the xfail mark in the same
commit that closes the issue. That is the right state machine: the test
flips from "bug exists" to "regression guard" in one atomic step.

Confirmed by running locally against master (commit 37af4ba):

test/test_alignment_contract.py xxxxxx [100%]
============================== 6 xfailed in 0.45s ===============================

Notes for maintainers

  • Please push back on any of the "expected behaviour" assertions before merge
    — pinning the contract is the point of this PR, so disagreement here is
    exactly the conversation we need. In particular: the divide test pins both
    "/ aligns by label" and "x / a == x * (1 / a)". If you'd rather not
    commit to the algebraic identity, that line is easy to drop.
  • The tests are deliberately small (≤30 lines each), share two minimal
    fixtures, and use only pd.Series / xr.DataArray — no solver call, no
    network I/O. They run in well under a second.
  • If you'd prefer this content to live inside test/test_constraints.py's
    existing TestConstraintCoordinateAlignment class or alongside
    test/test_variables.py, happy to relocate — kept it in a dedicated file
    to make the contract-pinning intent obvious.

Related issues: #586, #706, #707, #708, #709 (and #591, which proposes the
strict-alignment convention these tests would lock in).

@FBumann

Copy link
Copy Markdown
Collaborator

@MaykThewessen thanks for the PR. Ill add a bunch of similar tests soon. Could you try to cover more datatypes etc with pytest.parameterize, while keeping the test themselves and the parameters readable?

PyPSA#709
Add xfail(strict=True) regression tests for the coordinate-alignment bug
cluster. Each test asserts the expected behaviour (label-aligned operators,
coords-driven dimensionality in add_variables) so the contract is locked in
test form, and so the fix can flip an xfail to a regression guard in one
commit.
PyPSA#586's original add_constraints repro no longer fires on master (the path
now label-aligns correctly); the surviving arithmetic piece is covered by
the PyPSA#707 / PyPSA#708 tests.
@MaykThewessen
MaykThewessenforce-pushed the test/alignment-contract-tests branch from ce0db80 to 8ae16d7CompareMay 21, 2026 18:15
@MaykThewessen

Copy link
Copy Markdown
ContributorAuthor

Thanks @FBumann — pushed an update (force-with-lease, ce0db808ae16d7) that parametrizes the six tests over labelled coefficient / bound containers and dtypes. 6 cases → 21.

Coverage matrix:

TestIssueParametrize over
test_subtract_then_le_matches_direct_le#707pd.Series, xr.DataArray × float64, int64 (4)
test_add_aligns_by_label#708same (4)
test_subtract_aligns_by_label#708same (4)
test_divide_aligns_by_label#708same (4)
test_add_variables_dataarray_bounds_preserve_coords_order#706xr.DataArray × float64, int64 (2)
test_add_variables_pandas_bounds_broadcast_missing_dim#709pd.Series × float64, int64, pd.DataFrame × float64 (3)

All 21 parameter cases still xfail(strict=True) against master — confirmed locally:

test/test_alignment_contract.py ......x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x...x [100%]
============================= 21 xfailed in 1.29s ==============================

Readability choices:

  • Shared module-level constants (_COEF_LABELS, _VAR_LABELS, _X, _Y) so each parameter case is built by a one-line helper (_series("float64"), _dataarray("int64"), …) — the parameter IDs read as series-float64 / dataarray-int64 / dataframe-float64 rather than the auto-generated repr soup.
  • xfail mark stays on the test, not on each parameter case — every dtype/container combination reproduces the bug today, so a per-case mark would just be six identical reasons. The moment the upstream fix lands, all cases for a given test will XPASS together and the strict-mark forces removal in the same commit.
  • Unlabelled inputs (np.ndarray, list, scalar) are intentionally not parametrized — they have no labels for the contract to align against, so positional behaviour is correct for them and they would silently turn into XPASS noise. I left a note in the module docstring explaining this so it's not re-litigated in review.

Happy to widen further (e.g. xr.Dataset, ND DataArray, mixed dtypes via complex128 / Int64 extension) if you'd like — wasn't sure where the readability budget ran out. Also happy to fold these into your follow-up suite if you'd rather keep this PR minimal and land the broader parametrization in your own commit.

@FBumann

Copy link
Copy Markdown
Collaborator

@MaykThewessen Feel free to share your thoughts on #717

@FBumann

Copy link
Copy Markdown
Collaborator

THanks for the PR.
Those are now folded in and superseeded in #722 and #717. I validated that those are caught there.

@FBumannFBumann closed this May 25, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@MaykThewessen@FBumann