Linear interp with NaNs in nd indexer

[jenssss]

• Test added

When doing linear interpolatiion with an nd indexer that contains NaN's, xarray previously threw a KeyError from the missing._localize function. This PR fixes this by swapping np.min and np.max with np.nanmin and np.nanmax in that function, ignoring any NaN values.

[fujiisoup]

Thanks, @jenssss for sending a PR.
This looks good to me.
Could you add a line for this contribution to our whatsnew?

[fujiisoup]

It looks that np.nanmin (and nanmax) supports np.datetime-dtype only with numpy>=1.18.
We can copy np.nanmin to our core/npcompat.py and call this function here.

[mathause]

I think they added some low level loops to fix this (numpy/numpy#14841) so guess we cannot copy nanmin over and have to use if LooseVersion(np. __version__) < LooseVersion("1.18") (or similar).

[jenssss]

I added some LooseVersion checks for numpy>=1.18 in the d044142 commit to the missing._localize function and to the test. Would this do?

[keewis]

@jenssss, there's something wrong with the commit history (which makes reviewing pretty hard). Did you by change do git rebase upstream/master followed by a rebase pull?

[jenssss]

@keewis, sorry I'm still kinda new to Github.
Yeah, I did a rebase from upstream/master, and then a pull before pushing back up.

[keewis]

not your fault, the contributing guide did tell you to do that, and I only updated it a few days ago (not in stable yet). Do you want to try to fix that, or should I do it for you?

[jenssss]

I want to try to fix it.

[jenssss]

Okay, I think that should do it. How does it look now?

[keewis]

perfect, thanks for the fix.

[mathause]

Looks good - if you want to go a step further you can probably do something along the lines of (untested):

if new_x.dtype in "mM" and LooseVersion(np.__version__) < LooseVersion("1.18") and new_x.isnull().any():
    raise ValueError("numpy 1.18 or newer required to use interp with datetime/ timedelta array containing missing values")
else:
    imin = ... np.nanmin(...)
    imax = ...

[mathause]

I misremembered the behavior of the old np.min with datetimes - see this comment: https://github.com/pydata/xarray/pull/3924/files#discussion_r407101544

Sorry for sending you down the wrong path. I can have another look tomorrow.

[jenssss]

Okay, I removed the ValueError from my previous commit, so the content of _localize is now basically the same as in https://github.com/pydata/xarray/pull/3924/files#discussion_r407101544

I thought it better to use np.issubdtype(new_x.dtype, np.datetime64) instead of new_x.dtype.kind in "mM" to check for datetime type. It's a bit longer, but I think it makes it more readable since you don't have to look up what dtype.kind is.

[mathause]

Thanks! I have two more suggestions:

Can you add an additional line with

(["2000-01-01T12:00", "2000-01-02T12:00", "NaT"], [0.5, 1.5]), 

here:

xarray/xarray/tests/test_interp.py

Line 575 in 8313c3e

(["2000-01-01T12:00", "2000-01-02T12:00"], [0.5, 1.5]),

to have a test with a missing datetime.

Do you also want to add a note to the docstring, e.g. adding Missing values are skipped. Here:

xarray/xarray/core/dataarray.py

Line 1425 in a36d0a1

used for the broadcasting.

here:

xarray/xarray/core/dataarray.py

Line 1495 in a36d0a1

which to index the variables in this dataset.

Does it also work for DataSets? Then maybe add to the docstring here:

xarray/xarray/core/dataset.py

Line 2603 in a36d0a1

used for the broadcasting.

and here:

xarray/xarray/core/dataset.py

Line 2731 in a36d0a1

which to index the variables in this dataset.

[mathause]

LGTM. I'll merge in a day or two unless someone else has a comment. (#3924)

[mathause]

thanks @jenssss I see this is your first PR - welcome to xarray!