Skip to content

Commit 570fd6f

Browse files
FBumannclaude
andauthored
refactor(common): clarify coords-entry rules and tighten error labels (#733)
* refactor(common): clarify coords-entry rules and tighten error labels 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> * feat(common): allow dims= to name an unnamed pd.MultiIndex 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> * refactor(common): scope tuple-normalize check to lists/tuples with tuple 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 280f77c commit 570fd6f

4 files changed

Lines changed: 176 additions & 49 deletions

File tree

doc/release_notes.rst

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ Most users should keep calling ``model.solve(...)``. If you want more control, y
5252

5353
**Bug Fixes**
5454

55-
* ``Model.add_variables``: 0.7.0 made ``coords`` (dims, order, and values) the source of truth for ``DataArray`` bounds; this release closes the two remaining gaps. Pandas ``Series`` / ``DataFrame`` bounds missing a dimension are broadcast to ``coords`` instead of being silently dropped (`#709 <https://github.com/PyPSA/linopy/issues/709>`__), and the variable's dimension order always follows ``coords`` regardless of bound type (`#706 <https://github.com/PyPSA/linopy/issues/706>`__).
55+
* ``Model.add_variables``: 0.7.0 made ``coords`` (dims, order, and values) the source of truth for ``DataArray`` bounds; this release closes the two remaining gaps. Pandas ``Series`` / ``DataFrame`` bounds missing a dimension are broadcast to ``coords`` instead of being silently dropped (`#709 <https://github.com/PyPSA/linopy/issues/709>`__), and the variable's dimension order always follows ``coords`` regardless of bound type (`#706 <https://github.com/PyPSA/linopy/issues/706>`__). Bare-tuple coord entries (``coords=[(0, 1, 2)]``) now behave like lists.
5656
* ``add_variables`` / ``add_constraints``: the same rule now applies to ``mask`` — pandas ``Series`` / ``DataFrame`` masks missing a dimension are broadcast to the variable/constraint shape. As previously announced via ``FutureWarning``, masks whose coordinates are a sparse subset of the data's coordinates now raise ``ValueError`` rather than silently filling missing entries with ``False``; masks with dimensions not in the data raise ``ValueError`` instead of ``AssertionError``.
5757
* ``add_piecewise_formulation`` now produces a reproducible dimension order in the broadcast breakpoint array. The previous set-based expansion gave a hash-randomized order that varied between processes.
5858
* SOS constraints on masked variables no longer cause solver-specific failures (Gurobi ``IndexError``, Xpress ``?404 Invalid column number``, LP parse errors, silent set corruption). ``Model.solve()`` and ``Model.to_file()`` now raise a clear ``NotImplementedError`` referring users to `#688 <https://github.com/PyPSA/linopy/issues/688>`__; pass ``reformulate_sos=True`` as a workaround.

linopy/common.py

Lines changed: 54 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,11 @@ def as_dataarray(
318318
if coords is None:
319319
return _as_dataarray_lax(arr, coords, dims, **kwargs)
320320

321+
if isinstance(coords, list | tuple) and any(isinstance(c, tuple) for c in coords):
322+
# xarray reads bare `(a, b)` as `(dim_name, values)`; normalize so a
323+
# coords entry passed as a tuple behaves identically to a list.
324+
coords = [list(c) if isinstance(c, tuple) else c for c in coords]
325+
321326
expected = _coords_to_dict(coords, dims=dims)
322327
if not expected:
323328
return _as_dataarray_lax(arr, coords, dims, **kwargs)
@@ -478,8 +483,11 @@ def align_to_coords(
478483
Used by :meth:`~linopy.model.Model.add_variables` for ``lower``, ``upper``,
479484
and ``mask``, and by :meth:`~linopy.model.Model.add_constraints` for
480485
``mask``. Raises :class:`ValueError` with a message that names ``label``
481-
when conversion or validation fails.
486+
when ``value`` cannot be aligned to ``coords``. Coords-parsing errors
487+
propagate unchanged.
482488
"""
489+
if coords is not None:
490+
_coords_to_dict(coords, dims=kwargs.get("dims"))
483491
try:
484492
da = as_dataarray(value, coords, **kwargs)
485493
except TypeError as err:
@@ -497,21 +505,41 @@ def _coords_to_dict(
497505
"""
498506
Normalize coords to a dict mapping dim names to coordinate values.
499507
500-
For ``xarray.Coordinates`` (and ``DataArray.coords``), only entries
501-
that are actual dimensions are kept; derived MultiIndex level coords
502-
are dropped here and re-attached by xarray downstream. Plain mappings
503-
are returned as-is. For sequence inputs, entries must be ``pd.Index``
504-
(named or not) or unnamed sequences (``list`` / ``tuple`` / ``range``
505-
/ ``np.ndarray``). A ``pd.MultiIndex`` must have ``.name`` set —
506-
xarray requires a single dimension name for the flattened index.
507-
Other types — notably ``xarray.DataArray`` — raise ``TypeError``
508-
rather than being silently dropped: callers should convert via
509-
``variable.indexes[<dim>]`` (or ``pd.Index(...)``) first.
510-
511-
Unnamed sequence entries (or unnamed ``pd.Index``) gain a dim name
512-
from ``dims`` by position when ``dims`` is provided, so callers that
513-
pass ``coords=[[1, 2, 3]], dims=["x"]`` get the same strict
514-
enforcement as ``coords={"x": [1, 2, 3]}``.
508+
Container forms:
509+
510+
- ``xarray.Coordinates`` → kept dim entries only (MultiIndex level
511+
coords dropped).
512+
- ``Mapping`` → returned as a shallow ``dict`` copy.
513+
- sequence-of-entries → each entry handled per the rules below.
514+
515+
Sequence-entry rules (``i`` is the position in ``coords``, ``dims[i]``
516+
is the matching entry in ``dims`` when one exists). An entry is
517+
*unlabeled* if it's an unnamed ``pd.Index`` or a bare ``list`` /
518+
``tuple`` / ``range`` / ``ndarray``.
519+
520+
+---------------------------------+-----------------------+-----------+
521+
| Entry | Naming source | Outcome |
522+
+=================================+=======================+===========+
523+
| ``pd.Index`` with ``.name`` | ``.name`` | accepted |
524+
+---------------------------------+-----------------------+-----------+
525+
| unlabeled entry | ``dims[i]`` | accepted |
526+
+---------------------------------+-----------------------+-----------+
527+
| unlabeled entry | — (no ``dims[i]``) | skipped |
528+
| | | — xarray |
529+
| | | assigns |
530+
| | | ``dim_0`` |
531+
| | | etc. |
532+
+---------------------------------+-----------------------+-----------+
533+
| ``pd.MultiIndex`` with ``.name``| ``.name`` | accepted |
534+
+---------------------------------+-----------------------+-----------+
535+
| ``pd.MultiIndex`` w/o ``.name`` | ``dims[i]`` | accepted |
536+
| | | (named on |
537+
| | | a copy) |
538+
+---------------------------------+-----------------------+-----------+
539+
| ``pd.MultiIndex`` w/o ``.name`` | — (no ``dims[i]``) | TypeError |
540+
+---------------------------------+-----------------------+-----------+
541+
| anything else (e.g. DataArray) | — | TypeError |
542+
+---------------------------------+-----------------------+-----------+
515543
"""
516544
if isinstance(coords, Coordinates):
517545
# Coordinates iterates over every coord variable, including
@@ -525,13 +553,20 @@ def _coords_to_dict(
525553
result: dict[Hashable, Any] = {}
526554
for i, c in enumerate(coords):
527555
if isinstance(c, pd.MultiIndex):
528-
if not c.name:
556+
name = c.name or (
557+
dim_names[i] if dim_names and i < len(dim_names) else None
558+
)
559+
if name is None:
529560
raise TypeError(
530561
"MultiIndex coords entries must have .name set so "
531562
"xarray can use it as the dimension name. Set it via "
532-
"`idx.name = 'my_dim'` before passing to coords."
563+
"`idx.name = 'my_dim'`, or pass `dims=[...]` to name "
564+
"entries by position."
533565
)
534-
result[c.name] = c
566+
if c.name is None:
567+
c = c.copy()
568+
c.name = name
569+
result[name] = c
535570
elif isinstance(c, pd.Index):
536571
name = (
537572
c.name
@@ -577,32 +612,6 @@ def _named_pandas_to_dataarray(arr: pd.Series | pd.DataFrame) -> DataArray | Non
577612
return arr.to_xarray()
578613

579614

580-
def broadcast_mask(mask: DataArray, labels: DataArray) -> DataArray:
581-
"""
582-
Broadcast a boolean mask to match the shape of labels.
583-
584-
Ensures that mask dimensions are a subset of labels dimensions, broadcasts
585-
the mask accordingly, and fills any NaN values (from missing coordinates)
586-
with False while emitting a FutureWarning.
587-
"""
588-
assert set(mask.dims).issubset(labels.dims), (
589-
"Dimensions of mask not a subset of resulting labels dimensions."
590-
)
591-
mask = mask.broadcast_like(labels)
592-
if mask.isnull().any():
593-
warn(
594-
"Mask contains coordinates not covered by the data dimensions. "
595-
"Missing values will be filled with False (masked out). "
596-
"In a future version, this will raise an error. "
597-
"Use mask.reindex() or `linopy.align()` to explicitly handle missing "
598-
"coordinates.",
599-
FutureWarning,
600-
stacklevel=3,
601-
)
602-
mask = mask.fillna(False).astype(bool)
603-
return mask
604-
605-
606615
# TODO: rename to to_pandas_dataframe
607616
def to_dataframe(
608617
ds: Dataset,

test/test_common.py

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
"""
77

88
from collections.abc import Callable
9+
from typing import Any
910

1011
import numpy as np
1112
import pandas as pd
@@ -560,6 +561,124 @@ def test_align_to_coords_preserves_type_errors() -> None:
560561
align_to_coords(lambda x: x, {"x": [0, 1, 2]}, label="lower bound")
561562

562563

564+
def test_align_to_coords_does_not_relabel_coords_errors() -> None:
565+
"""Coords-side TypeError carries its own message, not the value label."""
566+
mi = pd.MultiIndex.from_product([[0, 1], ["a", "b"]], names=["i", "j"])
567+
with pytest.raises(TypeError, match=r"MultiIndex.*must have \.name set"):
568+
align_to_coords(np.array([1, 2, 3, 4]), [mi], label="lower bound")
569+
570+
571+
class TestCoordsToDictRules:
572+
"""
573+
One test per row of the ``_coords_to_dict`` rules table.
574+
575+
Each test name states the rule it pins; the assertions show the
576+
expected outcome. Together they form the executable spec of how
577+
sequence-form ``coords`` entries are named.
578+
"""
579+
580+
@staticmethod
581+
def _parse(coords: Any, dims: Any = None) -> dict:
582+
from linopy.common import _coords_to_dict
583+
584+
return _coords_to_dict(coords, dims=dims)
585+
586+
# -- container forms ---------------------------------------------------
587+
588+
def test_mapping_is_returned_as_shallow_dict_copy(self) -> None:
589+
src = {"x": [0, 1, 2], "y": [10, 20]}
590+
result = self._parse(src)
591+
assert result == src
592+
assert result is not src
593+
594+
def test_xarray_coordinates_keeps_only_dim_entries(self) -> None:
595+
midx = pd.MultiIndex.from_product([[0, 1], ["a", "b"]], names=["i", "j"])
596+
coords = xr.Coordinates.from_pandas_multiindex(midx, "stacked")
597+
result = self._parse(coords)
598+
assert set(result) == {"stacked"}
599+
600+
# -- pd.Index entries --------------------------------------------------
601+
602+
def test_named_pd_index_uses_its_name(self) -> None:
603+
result = self._parse([pd.Index([0, 1, 2], name="x")])
604+
assert set(result) == {"x"}
605+
606+
def test_unnamed_pd_index_with_dims_uses_dims(self) -> None:
607+
result = self._parse([pd.Index([0, 1, 2])], dims=["x"])
608+
assert set(result) == {"x"}
609+
610+
def test_unnamed_pd_index_without_dims_is_size_only(self) -> None:
611+
# Same as a bare sequence: contributes no dim name; xarray assigns
612+
# ``dim_0`` downstream.
613+
assert self._parse([pd.Index([0, 1, 2])]) == {}
614+
m = Model()
615+
v = m.add_variables(coords=[pd.Index([0, 1, 2])])
616+
assert v.dims == ("dim_0",)
617+
618+
# -- pd.MultiIndex entries --------------------------------------------
619+
620+
def test_named_multiindex_uses_its_name(self) -> None:
621+
mi = pd.MultiIndex.from_product([[0, 1], ["a", "b"]], names=["i", "j"])
622+
mi.name = "multi"
623+
result = self._parse([mi])
624+
assert set(result) == {"multi"}
625+
626+
def test_unnamed_multiindex_with_dims_uses_dims(self) -> None:
627+
mi = pd.MultiIndex.from_product([[0, 1], ["a", "b"]], names=["i", "j"])
628+
result = self._parse([mi], dims=["multi"])
629+
assert set(result) == {"multi"}
630+
assert result["multi"].name == "multi"
631+
assert mi.name is None # caller's MultiIndex not mutated
632+
633+
def test_unnamed_multiindex_without_dims_raises(self) -> None:
634+
mi = pd.MultiIndex.from_product([[0, 1], ["a", "b"]], names=["i", "j"])
635+
with pytest.raises(TypeError, match=r"MultiIndex.*must have \.name set"):
636+
self._parse([mi])
637+
638+
# -- bare sequence entries --------------------------------------------
639+
640+
@pytest.mark.parametrize(
641+
"entry",
642+
[[0, 1, 2], (0, 1, 2), range(3), np.array([0, 1, 2])],
643+
ids=["list", "tuple", "range", "ndarray"],
644+
)
645+
def test_bare_sequence_with_dims_uses_dims(self, entry: Any) -> None:
646+
result = self._parse([entry], dims=["x"])
647+
assert set(result) == {"x"}
648+
649+
@pytest.mark.parametrize(
650+
"entry",
651+
[[0, 1, 2], (0, 1, 2), range(3), np.array([0, 1, 2])],
652+
ids=["list", "tuple", "range", "ndarray"],
653+
)
654+
def test_bare_sequence_without_dims_is_silently_skipped(self, entry: Any) -> None:
655+
assert self._parse([entry]) == {}
656+
657+
@pytest.mark.parametrize(
658+
"entry",
659+
[[0, 1, 2], (0, 1, 2), range(3), np.array([0, 1, 2])],
660+
ids=["list", "tuple", "range", "ndarray"],
661+
)
662+
def test_bare_sequence_without_dims_falls_through_to_xarray_dim_0(
663+
self, entry: Any
664+
) -> None:
665+
m = Model()
666+
v = m.add_variables(coords=[entry])
667+
assert v.dims == ("dim_0",)
668+
669+
# -- unsupported entries ----------------------------------------------
670+
671+
def test_dataarray_entry_raises(self) -> None:
672+
with pytest.raises(TypeError, match=r"coords entries must be pd\.Index"):
673+
self._parse([DataArray([0, 1, 2], dims=["x"])])
674+
675+
def test_unknown_type_entry_raises(self) -> None:
676+
class Foo: ...
677+
678+
with pytest.raises(TypeError, match=r"coords entries must be pd\.Index"):
679+
self._parse([Foo()])
680+
681+
563682
def test_best_int() -> None:
564683
# Test for int8
565684
assert best_int(127) == np.int8

test/test_variable.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -499,11 +499,10 @@ def test_positional_bound_wrong_size_raises_clear_error(
499499
with pytest.raises(ValueError, match=r"upper bound could not be aligned"):
500500
model.add_variables(upper=pd.Series([1, 2]), coords=coords, name="s_bad")
501501

502-
def test_unnamed_coords_short_circuit(self, model: "Model") -> None:
503-
"""Coords as a list of unnamed indexes leaves the bound unchanged."""
502+
def test_unnamed_pd_index_is_size_only(self, model: "Model") -> None:
504503
bound = DataArray([1, 2, 3], dims=["dim_0"])
505504
var = model.add_variables(upper=bound, coords=[pd.Index([0, 1, 2])], name="x")
506-
assert (var.data.upper == [1, 2, 3]).all()
505+
assert (var.upper == [1, 2, 3]).all()
507506

508507
# -- Broadcasting missing dims -----------------------------------------
509508

0 commit comments

Comments
 (0)