Skip to content

refactor(common): clarify coords-entry rules and tighten error labels#733

Merged
FBumann merged 3 commits into
fix/bounds-coords-broadcastfrom
fix/strict-unnamed-index-and-cleanup
May 27, 2026
Merged

refactor(common): clarify coords-entry rules and tighten error labels#733
FBumann merged 3 commits into
fix/bounds-coords-broadcastfrom
fix/strict-unnamed-index-and-cleanup

Conversation

@FBumann
Copy link
Copy Markdown
Collaborator

@FBumann FBumann commented May 27, 2026

Stacks on top of #732. Small follow-ups from review.

What changes

  • Remove dead broadcast_maskrefactor: unify coords-as-truth handling in add_variables/add_constraints #732 claimed it was removed, but the function was still present in linopy/common.py with the old FutureWarning semantics that the rest of the PR explicitly replaced.
  • Tuple → list normalization in sequence-form coords. coords=[(0, 1, 2)] previously slipped through _coords_to_dict as a size-only entry but then tripped xarray's (dim_name, values) tuple convention with a confusing error. It now behaves identically to coords=[[0, 1, 2]].
  • align_to_coords no longer relabels coords-parsing errors. A TypeError from a bad coords entry (e.g. a stray xarray.DataArray) previously came out as "lower bound could not be aligned to coords: ...", misdirecting users to inspect the bound. It now propagates with its original message; only failures converting value get the labeled wrapper.
  • dims= can name an unnamed pd.MultiIndex (parity with pd.Index). The flat .name xarray needs is set on a shallow copy, so the caller's MultiIndex is not mutated. An unnamed MultiIndex with no positional dims= still raises.

Behavior table

This is the spec the docstring of _coords_to_dict and the TestCoordsToDictRules test class both follow (one test per row).

Container forms (top-level coords):

coords is... Behavior
xarray.Coordinates Kept dim entries only (MultiIndex level coords dropped).
Mapping Returned as a shallow dict copy.
sequence of entries Each entry handled per the rules below.

Sequence-entry rules (i is the position; dims[i] is the matching dims= entry when one exists). An entry is unlabeled if it's an unnamed pd.Index or a bare list / tuple / range / ndarray.

Entry Naming source Outcome
pd.Index with .name .name accepted
unlabeled entry dims[i] accepted
unlabeled entry — (no dims[i]) skipped — xarray assigns dim_0 etc. downstream
pd.MultiIndex with .name .name accepted
pd.MultiIndex without .name dims[i] accepted (named on a copy, caller's MI untouched)
pd.MultiIndex without .name — (no dims[i]) TypeError
anything else (e.g. DataArray) TypeError

Tests

TestCoordsToDictRules in test/test_common.py mirrors the table one-test-per-rule (22 tests, parametrized over list / tuple / range / ndarray for the bare-sequence cases). Each test name is the rule, so the executable spec stays visibly aligned with the docstring.

Test plan

  • pytest test/test_common.py::TestCoordsToDictRules — 22 passed
  • Full non-solver suite — 1978 passed, 1 skipped
  • Doctests on linopy/common.py and linopy/model.py — pass
  • Pre-commit (ruff/format/blackdoc/codespell) — clean

🤖 Generated with Claude Code

FBumann and others added 3 commits May 27, 2026 13:40
Stacks on top of #732. Three small follow-ups from PR review:

- Remove dead `broadcast_mask` (claimed removed in #732, was still present).
- `as_dataarray`: normalize bare-tuple coord entries to lists so
  `coords=[(0, 1, 2)]` behaves identically to `coords=[[0, 1, 2]]`
  (xarray reads `(a, b)` as `(dim_name, values)` and would otherwise
  raise a confusing error).
- `align_to_coords`: pre-validate coords via `_coords_to_dict` so
  TypeErrors from a bad `coords` argument propagate with their own
  message instead of being relabeled "<label> could not be aligned to
  coords: ...", which previously misdirected users to inspect the
  bound/mask.

Docs: replace the prose paragraph in `_coords_to_dict`'s docstring
with an explicit rules table covering every container form and
sequence-entry case (named/unnamed `pd.Index`, `pd.MultiIndex`, bare
sequences, with/without positional `dims=`).

Tests: new `TestCoordsToDictRules` class in `test_common.py` mirrors
the docstring table one-test-per-rule so the executable spec stays
visibly aligned with the documented contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the existing rule for unnamed pd.Index: an unnamed MultiIndex
paired with a positional dims=[i] entry now gets its flat .name set
to dims[i] on a shallow copy (caller's MultiIndex is not mutated).
Per-level names are preserved.

Removes the asymmetry between Index and MultiIndex in _coords_to_dict:
both can now be named either inline (.name) or by position (dims=[i]).
An unnamed MultiIndex with no positional dims still raises TypeError
since xarray requires a single flat name.

Adds one rule-table row and two tests
(test_unnamed_multiindex_with_dims_uses_dims,
test_unnamed_multiindex_without_dims_raises).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ple entries

The previous `not isinstance(coords, Coordinates | Mapping)` form was
broad and rebuilt `coords` as a fresh list on every call (even when no
tuple entries were present). Switch to a positive
`isinstance(coords, list | tuple)` guard with a short-circuit
`any(isinstance(c, tuple) for c in coords)` check, so the comprehension
only runs when there is actually a tuple to normalize.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@FBumann FBumann merged commit 570fd6f into fix/bounds-coords-broadcast May 27, 2026
2 of 3 checks passed
@FBumann FBumann deleted the fix/strict-unnamed-index-and-cleanup branch May 27, 2026 12:23
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