Numpy 1.18 support

[crusaderky]

Fix mean() and nanmean() for datetime64 arrays on numpy backend when upgrading from numpy 1.17 to 1.18.
All other nan-reductions on datetime64s were broken before and remain broken.
mean() on datetime64 and dask was broken before and remains broken.

• Closes test failure with upstream-dev #3409
• Passes black . && mypy . && flake8
• Fully documented, including whats-new.rst for all changes and api.rst for new API

[crusaderky]

xref #3434

[crusaderky]

Ready for review and merge

[mathause]

Given numpy/numpy#14841 - is this worth the extra complexity?

Currently xarray is only broken wrt numpy master and there is a chance numpy/numpy#14841 gets into 1.18 - then we could just use np.nanmin?

Also - is this the only place of min for datetimes? Should this also go into the operator dispatcher?

[dopplershift]

IMO, it's always best to release code that you know will work rather than rely on upstream to get something into the next release in order for you not to be broken. I say that both as a downstream user of xarray and a library maintainer.

[dcherian]

LGTM. I think we should merge so we can get a release out.

[crusaderky]

@dopplershift fully agree.

is this the only place of min for datetimes?

This PR does not fix the nan-reductions for datetime arrays.

It very narrowly fixes the use case of mean() and nanmean() with a numpy backend, so that they work on numpy 1.18 as they did on 1.17. As a matter of fact they never worked with dask, were never tested, and still won't work after the PR.

To emphasize: I am not introducing any new feature or fixing bugs on numpy 1.17; I'm just getting rid of regressions on the upgrade from 1.17 to 1.18.

Specifically, the PR fixes this failing test:

xarray/xarray/tests/test_duck_array_ops.py

Lines 276 to 293 in aa876cf

	@arm_xfail
	@pytest.mark.parametrize("dask", [False, True])
	def test_datetime_reduce(dask):
	time = np.array(pd.date_range("15/12/1999", periods=11))
	time[8:11] = np.nan
	da = DataArray(np.linspace(0, 365, num=11), dims="time", coords={"time": time})
	if dask and has_dask:
	chunks = {"time": 5}
	da = da.chunk(chunks)
	actual = da["time"].mean()
	assert not pd.isnull(actual)
	actual = da["time"].mean(skipna=False)
	assert pd.isnull(actual)
	# test for a 0d array
	assert da["time"][0].mean() == da["time"][:1].mean()

which was not well formulated - despite its name, it doesn't test all reductions, only mean(), and in fact never runs on dask (the index of a dask array is not chunked).
I have now rewritten it for the sake of clarity.

If numpy/numpy#14841 makes into master before 1.18 is published, and if it actually delivers on making reductions work with NaT (I did not test it yet), I'll be happy to write a second PR and rework everything.
Even then, a switch of this sort will still be needed:

def datetime_nanmin(array, **kwargs):
    if LooseVersion(numpy.__version__) < "1.18":
        return np.min(array, **kwargs)
    else:
        return np.nanmin(array, **kwargs)

The actual code will be in fact more complicated because you also need to think about dask.

[max-sixty]

Agree—let's merge?

[mathause]

Ok, thanks for the clarification.

[crusaderky]

Updated PR description:

Fix mean() and nanmean() for datetime64 arrays on numpy backend when upgrading from numpy 1.17 to 1.18.
All other nan-reductions on datetime64s were broken before and remain broken.
mean() on datetime64 and dask was broken before and remains broken.

[crusaderky]

I just realised there are no tests at all for datetime_to_numeric(offset=None) on a dask backend. I suspect I broke something - I need to look at it better.

[pep8speaks]

Hello @crusaderky! 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! 🍻

Comment last updated at 2019-11-19 09:23:14 UTC

[crusaderky]

datetime_to_numeric(offset=None) never worked with dask to begin with - there's a big fat TODO in it about it. I rewrote most of the PR; now mean() works with dask.

Pre-empting an observation: yes, the nanmin function I wrote could be used to work around the shortcomings of both numpy and dask on datetime64. I don't think xarray should do it and IMHO the workaround currently implemented for mean() should never have been done to begin with.

Again, this PR only fixes the regression on numpy 1.18.

[crusaderky]

Ready for review and merge