issue_comments

Congrats @Illviljan !

@Illviljan I hadn't seen this is still open! Definitely merge!

@Illviljan if you can resolve the merge conflict, we can merge this!

As discussed with @keewis and the team, we'll merge this after the next point release which fixes a small pandas compatibility issue.

I've been following this and think it looks good. There's some complication because of the need to handle different dims, but holding the requirements constant I think this is great.

Any thoughts from others?

So why not force integers to begin with?

I agree! Good change!

Empty coords is False as well, that's the trick here:

Ah good point. I guess we could either unroll that into two or checks so mypy understands, or give mypy a hint, or add ignores to the typing

It's true that Variable doesn't have it. But a Variable can't use that code path since we check getattr(array_large, "coords", False).

Ah, then we can tell mypy this, I think just by switching to isinstance(array_large, Variable) (and in case it wasn't clear, it's also OK to skip some of these or leave them to another PR. They can be useful checks for things we miss, but we shouldn't be beholden to the linting)

The check can't be that simple though because DataArrays doesn't always include coords and then it doesn't make sense to use reindex_like. I suppose adding a hasattr(array_large, "reindex_like") is an option, but it feels redundant to me.

getattr(array_large, "coords", False) will always pass for DataArrays — even if there are no coords, it's checking whether there's a coords method on the class — which in the case of a DataArray with no coords, would return empty (but still return). Or am I misunderstanding?

It's true that Variable doesn't have it. But a Variable can't use that code path since we check getattr(array_large, "coords", False).

Ah, then we can tell mypy this, I think just by switching to isinstance(array_large, Variable)

(and in case it wasn't clear, it's also OK to skip some of these or leave them to another PR. They can be useful checks for things we miss, but we shouldn't be beholden to the linting)

Sorry, I've been away for a bit.

Hmmm, I'm not sure. One is simple:

diff diff --git a/xarray/core/computation.py b/xarray/core/computation.py index af2c37db..5a839c19 100644 --- a/xarray/core/computation.py +++ b/xarray/core/computation.py @@ -1400,7 +1400,7 @@ def _get_valid_values(da, other): def cross( a: "T_DSorDAorVar", b: "T_DSorDAorVar", - dim: str, + dim: Hashable, ) -> "T_DSorDAorVar": """ Return the cross product of two (arrays of) vectors.

But that leaves lots of others! I'll have to come back to this, or if others have ideas, gratefully received. Some quick thoughts of mediocre helpfulness:

xarray/core/computation.py:1576: error: Dict entry 0 has incompatible type "Hashable": "int"; expected "Hashable": "Union[None, Number, str, Tuple[Number, ...]]" [dict-item] xarray/core/computation.py:1576: error: Dict entry 0 has incompatible type "Hashable": "int"; expected "Hashable": "Union[None, Number, Tuple[Number, ...]]" [dict-item] xarray/core/computation.py:1536: error: Dict entry 0 has incompatible type "Hashable": "int"; expected "Hashable": "Union[None, Number, str, Tuple[Number, ...]]" [dict-item]

I don't get how Hashable: int isn't compatible with Hashable: Number??

xarray/core/computation.py:1533: error: Argument "sample_dims" to "to_stacked_array" of "Dataset" has incompatible type "Mapping[Hashable, int]"; expected "Mapping[Hashable, Sequence[Hashable]]" [arg-type]

Does this refer to Dataset.dims and DataArray.dims having different types, and that we may want to use sizes or coerce the .dims to a list?

xarray/core/computation.py:1570: error: "Variable" has no attribute "reindex_like" [attr-defined]

Is this legit — does it work with Variables?

xarray/core/computation.py:1532: error: No overload variant of "__setitem__" of "list" matches argument types "int", "DataArray" [call-overload] xarray/core/computation.py:1532: note: Possible overload variants: xarray/core/computation.py:1532: note: def __setitem__(self, int, Dataset) -> None xarray/core/computation.py:1532: note: def __setitem__(self, slice, Iterable[Dataset]) -> None xarray/core/computation.py:1532: error: Incompatible types in assignment (expression has type "DataArray", variable has type "Dataset") [assignment]

I'm not even sure where are these variants are? Is it referring to the Dataset.__setitem__? Surely that accepts DataArrays?

...and ironically these show up in the simpler area of the code rather than the complicated part that allows for passing 2 or 3 sized dims!

Found 7 errors in 1 file (checked 142 source files)

That's annoying, sorry @Illviljan . Re the failure, issue added here: https://github.com/pydata/xarray/issues/5444.

This is looking excellent @Illviljan ! Let us know when it's ready for a final review / merge

This looks really good, very nice code @Illviljan .

I admittedly haven't thought exhaustively through whether we're missing anything. But the tests are great.

FWIW the numpy API is not simple — the "length 2 or 3" seems to contribute more complication than value. If anyone thinks that we should instead enforce length 3, I would support that; but otherwise defaulting to the numpy API is good.