Skip to content

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

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

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

Issue Test Contract pinned
#707 test_subtract_then_le_matches_direct_le x - a <= 0 and x <= a build the same constraint
#708 test_add_aligns_by_label x + a aligns by label, not by position
#708 test_subtract_aligns_by_label x - a aligns by label
#708 test_divide_aligns_by_label x / a aligns by label and x / a == x * (1 / a)
#706 test_add_variables_dataarray_bounds_preserve_coords_order add_variables dim order follows coords for DataArray bounds with missing dims
#709 test_add_variables_pandas_bounds_broadcast_missing_dim add_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

FBumann commented May 21, 2026

@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 MaykThewessen force-pushed the test/alignment-contract-tests branch from ce0db80 to 8ae16d7 Compare May 21, 2026 18:15
@MaykThewessen
Copy link
Copy Markdown
Contributor Author

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:

Test Issue Parametrize over
test_subtract_then_le_matches_direct_le #707 pd.Series, xr.DataArray × float64, int64 (4)
test_add_aligns_by_label #708 same (4)
test_subtract_aligns_by_label #708 same (4)
test_divide_aligns_by_label #708 same (4)
test_add_variables_dataarray_bounds_preserve_coords_order #706 xr.DataArray × float64, int64 (2)
test_add_variables_pandas_bounds_broadcast_missing_dim #709 pd.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.

Sign up for free to 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