Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid invalid Coordinates object #8884

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion xarray/core/coordinates.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
assert_no_index_corrupted,
create_default_index_implicit,
)
from xarray.core.merge import merge_coordinates_without_align, merge_coords
from xarray.core.merge import (
assert_valid_explicit_coords,
merge_coordinates_without_align,
merge_coords,
)
from xarray.core.types import DataVars, Self, T_DataArray, T_Xarray
from xarray.core.utils import (
Frozen,
Expand Down Expand Up @@ -306,6 +310,11 @@ def __init__(
else:
variables[name] = var

dims = set(d for var in variables.values() for d in var.dims)
assert_valid_explicit_coords(
variables, dims=dims, explicit_coords=list(variables)
)

if indexes is None:
indexes = {}
else:
Expand Down
10 changes: 5 additions & 5 deletions xarray/core/merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -564,7 +564,7 @@ def merge_coords(

def assert_valid_explicit_coords(
variables: Mapping[Any, Any],
dims: Mapping[Any, int],
dims: Iterable[Any],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this because the lengths are never used, and renamed dims->sizes in a few places to avoid this confusion.

explicit_coords: Iterable[Hashable],
) -> None:
"""Validate explicit coordinate names/dims.
Expand Down Expand Up @@ -721,16 +721,16 @@ def merge_core(
collected, prioritized, compat=compat, combine_attrs=combine_attrs
)

dims = calculate_dimensions(variables)
sizes = calculate_dimensions(variables)

coord_names, noncoord_names = determine_coords(coerced)
if compat == "minimal":
# coordinates may be dropped in merged results
coord_names.intersection_update(variables)
if explicit_coords is not None:
assert_valid_explicit_coords(variables, dims, explicit_coords)
assert_valid_explicit_coords(variables, sizes.keys(), explicit_coords)
coord_names.update(explicit_coords)
for dim, size in dims.items():
for dim, size in sizes.items():
if dim in variables:
coord_names.add(dim)
ambiguous_coords = coord_names.intersection(noncoord_names)
Expand All @@ -745,7 +745,7 @@ def merge_core(
combine_attrs,
)

return _MergeResult(variables, coord_names, dims, out_indexes, attrs)
return _MergeResult(variables, coord_names, sizes, out_indexes, attrs)


def merge(
Expand Down
9 changes: 8 additions & 1 deletion xarray/tests/test_coordinates.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import numpy as np
import pandas as pd
import pytest

Expand All @@ -8,7 +9,7 @@
from xarray.core.dataarray import DataArray
from xarray.core.dataset import Dataset
from xarray.core.indexes import PandasIndex, PandasMultiIndex
from xarray.core.variable import IndexVariable
from xarray.core.variable import IndexVariable, Variable
from xarray.tests import assert_identical, source_ndarray


Expand Down Expand Up @@ -69,6 +70,12 @@ def test_init_dim_sizes_conflict(self) -> None:
with pytest.raises(ValueError):
Coordinates(coords={"foo": ("x", [1, 2]), "bar": ("x", [1, 2, 3, 4])})

def test_init_var_shares_name_with_dim(self) -> None:
# regression test for GH #8883
var = Variable(data=np.arange(6).reshape(2, 3), dims=["x", "y"])
with pytest.raises(ValueError):
Coordinates(coords={"x": var}, indexes={})

def test_from_pandas_multiindex(self) -> None:
midx = pd.MultiIndex.from_product([["a", "b"], [1, 2]], names=("one", "two"))
coords = Coordinates.from_pandas_multiindex(midx, "x")
Expand Down
Loading