issue_comments

I think we could bump the version even further (at least to 2.8, maybe also 2.9) so that should probably be a different PR

Do you need to add a note to what's new that the dask version is bumped?

unless there are any objections (@shoyer?) I'm going to merge this tomorrow.

gentle ping, @shoyer

anyway, with this the tests finally pass :tada: so this should be ready for review and merging.

then it seems that this PR removes this intentional restriction. I'm not sure it is still needed, though: we don't use np.testing.assert_allclose internally because we want to provide our own error messages, and the actual comparison is done with https://github.com/pydata/xarray/blob/e5cc19cd8f8a69e0743f230f5bf51b7778a0ff96/xarray/core/duck_array_ops.py#L204 which should allow dispatching

I'm confused, I thought that xr.testing.assert_allclose explicitly supported duck arrays (it calls duck_array_ops.allclose_or_equiv).

Right, it explicitly supports duck arrays -- by always converting them into NumPy!

hmm... using arr.compute() fails while np.array(arr) works. For now, I'm converting all dask arrays to numpy if the dask version is not high enough (I can't do that only for bool_ arrays since they have been converted to float somewhere).

@shoyer: I'm confused, I thought that xr.testing.assert_allclose explicitly supported duck arrays (it calls duck_array_ops.allclose_or_equiv). TBC, what I was talking about was these lines: https://github.com/pydata/xarray/blob/e5cc19cd8f8a69e0743f230f5bf51b7778a0ff96/xarray/testing.py#L124 https://github.com/pydata/xarray/blob/e5cc19cd8f8a69e0743f230f5bf51b7778a0ff96/xarray/testing.py#L132-L135 where the .values always convert to numpy

So it seems that assert_allclose never worked with dask but always converted to numpy. Would it make sense to investigate further, or is it better to just wait until we can merge this?

This was intentional, I think. np.testing.assert_allclose() doesn't support dispatching -- it always converts into NumPy arrays. I'm not sure that assert_allclose would even be well defined on dask arrays otherwise, because it doesn't have any output (it only raises an error).

let's see if that works

How about we call compute in assert_allclose for boolean dask arrays when dask < 2.9.1? This bit can then be removed in a couple of months.

This compute was happening in previous versions anyway (because assert_allclose was using .values) so this would not be a regression.

I needed to use assert_allclose in the pint tests in #3975, so I modified it to use .data instead of .values and now the exact same tests are failing. So it seems that assert_allclose never worked with dask but always converted to numpy. Would it make sense to investigate further, or is it better to just wait until we can merge this?

Would it make sense to wait until we can bump the dask version to 2.9?

Sounds good to me. It's only 1.5 months away

Unfortunately, simply replacing assert np.allclose(...) with np.testing.assert_allclose(...) does not fix this. Also, I undid the swapping of arguments to allclose_or_equiv which makes the min-all-deps CI fail, too. Does anyone know why that works?

Would it make sense to wait until we can bump the dask version to 2.9?

this comment: # Numpy < 1.13 does not handle object-type array. suggests that this code needs to be updated for our new numpy>=1.15 requirement

It looks like there might be some sort of issue with dask's numpy compatibility layer? e.g., in np.allclose?

These fallbacks are catching quite a few errors....

One idea would be to try plumbing this logic into np.testing.assert_allclose (which doesn't do dispatching) rather than np.allclose. The former comes pre-packaged with better error messages, which might be handy.

example failure: ``` ___ test_reduce[None-True-var-True-bool_-1] ______

dim_num = 1, dtype = <class 'numpy.bool_'>, dask = True, func = 'var' skipna = True, aggdim = None

@pytest.mark.parametrize("dim_num", [1, 2])
@pytest.mark.parametrize("dtype", [float, int, np.float32, np.bool_])
@pytest.mark.parametrize("dask", [False, True])
@pytest.mark.parametrize("func", ["sum", "min", "max", "mean", "var"])
# TODO test cumsum, cumprod
@pytest.mark.parametrize("skipna", [False, True])
@pytest.mark.parametrize("aggdim", [None, "x"])
def test_reduce(dim_num, dtype, dask, func, skipna, aggdim):

    if aggdim == "y" and dim_num < 2:
        pytest.skip("dim not in this test")

    if dtype == np.bool_ and func == "mean":
        pytest.skip("numpy does not support this")

    if dask and not has_dask:
        pytest.skip("requires dask")

    if dask and skipna is False and dtype in [np.bool_]:
        pytest.skip("dask does not compute object-typed array")

    rtol = 1e-04 if dtype == np.float32 else 1e-05

    da = construct_dataarray(dim_num, dtype, contains_nan=True, dask=dask)
    axis = None if aggdim is None else da.get_axis_num(aggdim)

    # TODO: remove these after resolving
    # https://github.com/dask/dask/issues/3245
    with warnings.catch_warnings():
        warnings.filterwarnings("ignore", "Mean of empty slice")
        warnings.filterwarnings("ignore", "All-NaN slice")
        warnings.filterwarnings("ignore", "invalid value encountered in")

        if da.dtype.kind == "O" and skipna:
            # Numpy < 1.13 does not handle object-type array.
            try:
                if skipna:
                    expected = getattr(np, f"nan{func}")(da.values, axis=axis)
                else:
                    expected = getattr(np, func)(da.values, axis=axis)

                actual = getattr(da, func)(skipna=skipna, dim=aggdim)
                assert_dask_array(actual, dask)
                assert np.allclose(
                    actual.values, np.array(expected), rtol=1.0e-4, equal_nan=True
                )
            except (TypeError, AttributeError, ZeroDivisionError):
                # TODO currently, numpy does not support some methods such as
                "casting='same_kind'"

              % (funcname(function), str(dtype), str(result.dtype))
            )

E ValueError: Inferred dtype from function 'sub' was 'int64' but got 'float64', which can't be cast using casting='same_kind' ```

I'm confused here. What did you change to trigger this error?

ALL WRONG: ~We could call arr1.compute(), arr2.compute() in https://github.com/pydata/xarray/blob/69548df9826cde9df6cbdae9c033c9fb1e62d493/xarray/core/duck_array_ops.py#L203-L204~

~At that point, we need to compare actual values. I'm not sure that dask exits all() early if one chunk evaluates to False ( in which case this change would be a performance regression)~

EDIT: ignore this. We can't compute because it might blow memory.

@pydata/xarray, this is really close but I'm not familiar enough with dask to get it to work with dask<2.9.1.

Once we get the py36-min-all-deps and py36-min-nep18 CI to pass, this should be ready for a final review and merging.

Edit: there seem to be a few failing tests related to pint (I will fix those) but the tests in question are in test_duck_array_ops.py and only fail with py36-min-nep18.

The issue is with how dask<2.9.1 handles dtypes on compute (in nanvar?) when the data is an array with dtype object filled with False and some missing values represented by np.nan.

I really lack experience with dask, though, so I'm clueless as to what to do to fix that (besides calling compute before passing to allclose_or_equiv) and would appreciate help with this.

unfortunately, we need dask>=2.9.1 for that. I could try debugging a bit more to find out exactly why it fails (or someone helps me with that?), but that would take me a bit more time than just skipping / xfailing those tests if on dask<2.9.1.

Our minimum versions policy allows us to bump dask. So let's do that? dask 2.2 (2019-08-01) 2.5 (2019-09-27) < distributed 2.2 (2019-08-01) 2.5 (2019-09-27) <

Also, where should I put the whats-new.rst entry?

New features? It is public API and is a very nice enhancement.

to me the failing tests on the min-all-deps and min-nep18 CI seem like a dask issue (in var?) that has been resolved in later versions: if we compute actual before passing it to assert_allclose the tests don't fail. How should I resolve this?

Also, where should I put the whats-new.rst entry?

now only the min-all-deps and min-nep18 CI fail. I suspect that is due to the pinned dependencies.

Let's mark those as xfail then?

the failures should be a bug in pint and hopefully fixed by hgrecco/pint#1044

Looks great!