issue_comments

Congrats @Illviljan !

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

I'm will merge this in a couple of days if no one minds.

Unit Test Results

6 files           6 suites   57m 11s :stopwatch: 16 304 tests 14 565 :heavy_check_mark: 1 739 :zzz: 0 :x: 91 020 runs  82 822 :heavy_check_mark: 8 198 :zzz: 0 :x:

Results for commit f2d98b61.

:recycle: This comment has been updated with latest results.

Hello @Illviljan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-10-31 08:51:03 UTC

@keewis this is ready for another review if you're up for it.

@max-sixty Had to use Union[DataArray, Variable] instead of Union[T_DataArray, T_Variable] from types.py. Apparently you can't narrow down the typing using isinstance if you use T_DataArray = TypeVar("T_DataArray", bound="DataArray"). Not sure why, I'm just accepting it at this point.

Typing errors: xarray/core/computation.py:1525: error: Item "Sequence[Sequence[Sequence[Sequence[Sequence[Any]]]]]" of "Union[DataArray, Variable, DataArrayGroupBy, Union[Union[Sequence[Sequence[Sequence[Sequence[Sequence[Any]]]]], Union[Union[_SupportsArray[dtype[Any]], Sequence[_SupportsArray[dtype[Any]]], Sequence[Sequence[_SupportsArray[dtype[Any]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]]]], Union[bool, int, float, complex, str, bytes, Sequence[Union[bool, int, float, complex, str, bytes]], Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]], Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]], Sequence[Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]]]]]], generic, ndarray[Any, Any], Any]]" has no attribute "dims" [union-attr] xarray/core/computation.py:1525: error: Item "_SupportsArray[dtype[Any]]" of "Union[DataArray, Variable, DataArrayGroupBy, Union[Union[Sequence[Sequence[Sequence[Sequence[Sequence[Any]]]]], Union[Union[_SupportsArray[dtype[Any]], Sequence[_SupportsArray[dtype[Any]]], Sequence[Sequence[_SupportsArray[dtype[Any]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]]]], Union[bool, int, float, complex, str, bytes, Sequence[Union[bool, int, float, complex, str, bytes]], Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]], Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]], Sequence[Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]]]]]], generic, ndarray[Any, Any], Any]]" has no attribute "dims" [union-attr] xarray/core/computation.py:1525: error: Item "Sequence[_SupportsArray[dtype[Any]]]" of "Union[DataArray, Variable, DataArrayGroupBy, Union[Union[Sequence[Sequence[Sequence[Sequence[Sequence[Any]]]]], Union[Union[_SupportsArray[dtype[Any]], Sequence[_SupportsArray[dtype[Any]]], Sequence[Sequence[_SupportsArray[dtype[Any]]]], Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]], Sequence[Sequence[Sequence[Sequence[_SupportsArray[dtype[Any]]]]]]], Union[bool, int, float, complex, str, bytes, Sequence[Union[bool, int, float, complex, str, bytes]], Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]], Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]], Sequence[Sequence[Sequence[Sequence[Union[bool, int, float, complex, str, bytes]]]]]]]], generic, ndarray[Any, Any], Any]]" has no attribute "dims" [union-attr] I think it's due to the ScalarOrArray type in DaCompatible. Not sure what part is supposed to be compatible though.

Hello, @Illviljan. on an unrelated matter would you mind sending me an email (deepak@cherian.net) for an offline chat.

@keewis, I've removed the ds support. Please check the example if it looks like you thought with .to_array, .to_dataset.

we discussed this in the meeting yesterday, and decided that since it makes the code cleaner and there's the dim parameter of to_array and to_dataset (which I didn't know about when I suggested the ugly to_unstacked_array hack) we would like to restrict .cross to accept only DataArray objects. Could you make that change?

To increase their visibility it would probably also be good to show how to use to_array and to_dataset to pass Dataset objects to .cross.

This however doesn't work as expected: python ds = xr.Dataset({"x": ("t", [0, 1]), "y": ("t", [2, 3]), "z": ("t", [4, 5])}) arr = xr.DataArray(data=[0, 2, 4], dims="cartesian", coords={"cartesian": ["x", "y", "z"]}) xr.cross(ds, arr, dim="cartesian") # result: DataArray, should've been Dataset Seems an easy fix fortunately.

The use case is something similar to: python ds = xr.Dataset({"x": ("t", [0, 1]), "y": ("t", [2, 3]), "z": ("t", [4, 5])}) arr = xr.DataArray(data=[0, 2, 4], dims="cartesian", coords={"cartesian": ["x", "y", "z"]}) xr.cross(arr, ds, dim="cartesian") # result: Dataset with x, y, z variables This does return what I want it to (which is why I don't think it is a bug), but my worry was that since ds does not have a dimension called "cartesian" it might be surprising that this works (though good documentation might help).

In any case, I'd like to get the opinion of other maintainers: is the integrated conversion worth the trouble, or should we make combining / splitting variables easier?

Hmm mightve found a bug there. Could you show the examples? my idea was that if there's any dataset as input the output should always be dataset. I think apply_ufunc or some other function has the prio order ds > da > var.

I finally got around to testing this. This works really well for the use cases I tried, but the automatic conversion of Datasets with three variables to a DataArray is a bit surprising: python xr.cross(arr, ds, dim="cartesian") # ← ds does not have a "cartesian" dimension not sure what to do about that. @pydata/xarray, any ideas?

To be clear, the idea was to have a convenient way to convert a Dataset with three identically dimensioned variables to a DataArray with an additional additional dimension. If there's a concise way to do that I would lean towards not doing that in cross() (even though I was the one to propose it). For example: python xr.cross( arr, ds.to_array(concat_dim="cartesian"), dim="cartesian" ).to_dataset(cut_dim="cartesian")

@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?

Green colors. :) I think this is finished now unless someone has any other thoughts.

So why not force integers to begin with?

I agree! Good change!

Integers don't work with Numbers: 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]

Floats don't work with Numbers: xarray/core/computation.py:1536: error: Dict entry 0 has incompatible type "Hashable": "int"; expected "Hashable": "Union[None, Number, str, Tuple[Number, ...]]" [dict-item] xarray/core/computation.py:1578: error: Dict entry 0 has incompatible type "Hashable": "float"; expected "Hashable": "Union[None, Number, str, Tuple[Number, ...]]" [dict-item] xarray/core/computation.py:1578: error: Dict entry 0 has incompatible type "Hashable": "float"; expected "Hashable": "Union[None, Number, Tuple[Number, ...]]" [dict-item] This seems related to: https://github.com/python/mypy/issues/3186

I do wonder why we allow any type of numbers in the chunks? When does it ever make sense to input 1.5 or a complex number? https://github.com/pydata/xarray/blob/28d3349bbe8c9881448bba45785c1013b006d6f9/xarray/core/dataarray.py#L1041

It will always be forced to integers anyway: https://github.com/dask/dask/blob/8aea537d925b794a94f828d35211a5da05ad9dce/dask/array/core.py#L2815 So why not force integers to begin with?

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

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?

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

```python a = xr.DataArray([1, 2]) if getattr(a, "coords", False): print("hey")

b = xr.DataArray([1, 2], coords=[[0, 1]]) if getattr(b, "coords", False): print("hey") hey ```

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?

mypy doesn't understand hasattr(array_large, "reindex_like")apparently: xarray/core/computation.py:1572: error: "Variable" has no attribute "reindex_like" [attr-defined]

Rather surprising that it's this difficult to make mypy happy. The code seems pretty straightforward to me.

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.

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)

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

Is this legit — does it work with Variables?

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).

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)

@max-sixty, any ideas how to fix these mypy errors?

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

@keewis, @max-sixty I think this ready now.

I don't get why my local version managed to pass without having to rechunk the datasets though but it's in the past now.

Well, look at that. Updating to latest I get the error as well. astroid 2.5.6-py38haa244fe_0 --> 2.5.7-py38haa244fe_0 ca-certificates 2020.12.5-h5b45459_0 --> 2021.5.30-h5b45459_0 certifi 2020.12.5-py38haa244fe_1 --> 2021.5.30-py38haa244fe_0 curl 7.76.1-h789b8ee_2 --> 7.77.0-h789b8ee_0 dask 2021.5.0-pyhd8ed1ab_0 --> 2021.6.0-pyhd8ed1ab_0 dask-core 2021.5.0-pyhd8ed1ab_0 --> 2021.6.0-pyhd8ed1ab_0 distributed 2021.5.0-py38haa244fe_0 --> 2021.6.0-py38haa244fe_0 docutils 0.16-py38haa244fe_3 --> 0.17.1-py38haa244fe_0 importlib-metadata 4.0.1-py38haa244fe_0 --> 4.5.0-py38haa244fe_0 importlib_metadata 4.0.1-hd8ed1ab_0 --> 4.5.0-hd8ed1ab_0 ipython 7.23.1-py38h43734a8_0 --> 7.24.1-py38h43734a8_0 jupyterlab_server 2.5.2-pyhd8ed1ab_0 --> 2.6.0-pyhd8ed1ab_0 libcurl 7.76.1-h789b8ee_2 --> 7.77.0-h789b8ee_0 liblief 0.11.4-h0e60522_0 --> 0.11.5-h0e60522_0 nbclassic 0.2.8-pyhd8ed1ab_0 --> 0.3.1-pyhd8ed1ab_1 networkx 2.5-py_0 --> 2.5.1-pyhd8ed1ab_0 pandoc 2.13-h8ffe710_0 --> 2.14.0.1-h8ffe710_0 prometheus_client 0.10.1-pyhd8ed1ab_0 --> 0.11.0-pyhd8ed1ab_0 py-lief 0.11.4-py38h885f38d_0 --> 0.11.5-py38h885f38d_0 pyerfa 1.7.3-py38h294d835_0 --> 1.7.3-py38hcb5f9af_1 python-libarchive~ 2.9-py38haa244fe_2 --> 3.1-py38haa244fe_0 pyzmq 22.0.3-py38h09162b1_1 --> 22.1.0-py38h09162b1_0 rope 0.18.0-pyhd3deb0d_0 --> 0.19.0-pyhd8ed1ab_0 sphinx 4.0.0-pyh6c4a22f_0 --> 4.0.2-pyh6c4a22f_1 sqlalchemy 1.4.15-py38h294d835_0 --> 1.4.17-py38h294d835_0 tqdm 4.60.0-pyhd8ed1ab_0 --> 4.61.0-pyhd8ed1ab_0 typing_extensions 3.7.4.3-py_0 --> 3.10.0.0-pyha770c72_0 urllib3 1.26.4-pyhd8ed1ab_0 --> 1.26.5-pyhd8ed1ab_0 xarray 0.18.0-pyhd8ed1ab_0 --> 0.18.2-pyhd8ed1ab_0

The errors with test_vectorize_dask_dtype_meta are unrelated to my changes and mentioned in #5366 as well. Can't replicate the cross errors on my local versions though, maybe the root cause is the same?

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.