issue_comments

see #3950

Merging: sure, I just wanted to avoid merging my own PRs without a final review

IMO @keewis you should merge this if you think its ready.

even if it's just to clarify why we accept different types in those functions: I think init may convert its arguments so it supports more types than array_ufunc (I don't know about the others, though).

open an issue for this?

Hello @keewis! 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 2020-04-21 16:59:32 UTC

Is part of the specific problem that Pint has a guarded import of xarray when defining its upcast types? Would it help if it checked for the fully qualified class name instead?

SupportsArithmetic is a base class of Variable, DataArray and Dataset, so if I try to import pint for type checking, pint can't use these classes, otherwise we'd have a circular import. Comparing fully qualified class names would work, but it might make sense to wait for a recommendation regarding my question.

Should this be its own issue?

Definitely, even if it's just to clarify why we accept different types in those functions: I think __init__ may convert its arguments so it supports more types than __array_ufunc__ (I don't know about the others, though). For the missing __array_function__ see #3917.

Also, after glancing through all this, it seems like xarray is dealing inconsistently with the type casting hierarchy:

• Construction/wrapping allowing __array_function__-defining duck arrays among other explictly listed additions (pandas.Index, dask arrays, etc., see https://github.com/pydata/xarray/blob/9b5140e0711247c373987b56726282140b406d7f/xarray/core/variable.py#L166)
• Binary ops only denying based on xarray's internal hierarchy (Dataset, DataArray, Variable), otherwise deferring to underlying data
• __array_ufunc__ allowing a list of supported types (https://github.com/pydata/xarray/blob/9b5140e0711247c373987b56726282140b406d7f/xarray/core/arithmetic.py#L24-L30 along with SupportsArithmetic)
• (__array_function__ yet to be defined)

Should this be its own issue?

@keewis Is part of the specific problem that Pint has a guarded import of xarray when defining its upcast types? Would it help if it checked for the fully qualified class name instead?

Ugh. Trying to add pint.Quantityto the list of handled types does not work since pint tries to add xarray to its known non-handled types – that means we have a circular import. I guess I'll xfail the bivariate_ufuncs tests, too.

To resolve this, the type registration mentioned in #1617 and https://github.com/pydata/xarray/blob/ba9f82227a9ec0c42928d5d0b978b101eda756cb/xarray/core/arithmetic.py#L23 might help?

see https://github.com/dask/dask/issues/5329#issuecomment-609453229. Basically, pint is high enough in the type hierarchy that there are too many types below it and it is easier for it to list those above it.

Actually, pint and xarray using different (conflicting) ways to check the support was the reason why I asked for advice in that issue.

pint tries to add xarray to its known non-handled types

I don't know much about this stuff can't pint have just a list of handled types and return NotImplementedfor anything else?

it seems we can't use __array_function__ to check if the type should be handled or not (using __array_function__ works, but that's by accident, I think, and stops working once/if we implement __array_function__on xarray objects).

~I'd say it is pretty rare the other type knows what to do with a xarray object, so I think we should retry using the wrapped object.~ I don't know the reason for the current implementation, though, so I could be missing something. For reference, this only is a problem with functions that accept multiple values, like numpy.maximum. Any thoughts on this, @shoyer?

Edit: actually, that smells like a type hierarchy issue, as discussed in hgrecco/pint#845 and dask/dask#5329

I put in an issue

thanks, @max-sixty.

not sure what the bot is checking, xarray/core/arithmetic.py doesn't have 1934 lines (the last line is 107) and black doesn't complain about indentation

not sure what the bot is checking, xarray/core/arithmetic.py doesn't have 1934 lines (the last line is 107) and black doesn't complain about indentation for any line

I put in an issue

LGTM! Thanks as ever @keewis

I updated #3594 with a list of libraries that won't work and opened the requested issue.

If there are no objections to ~using __array_function__ as a criterion for supported types in __array_ufunc__ (see https://github.com/pydata/xarray/pull/3643#discussion_r396101559) or~ any of the other changes, this should be ready to merge?

, but some of these seem to depend on DataArray.array_function which is not defined

Let's open an issue for visibility.

we have a few issues with non-numpy libraries (scipy, bottleneck, numbagg) and the semi-public numpy.lib.stride_tricks.as_strided and of course the indexing does not work either.

This seems OK for now.

I think this is almost ready. For remaining issues see https://github.com/pydata/xarray/pull/3643#issue-354872657: we have a few issues with non-numpy libraries (scipy, bottleneck, numbagg) and the semi-public numpy.lib.stride_tricks.as_strided and of course the indexing does not work either.

Also, I tried to add tests for using numpy functions on DataArray objects, but some of these seem to depend on DataArray.__array_function__ which is not defined (e.g. np.median, np.searchsorted, np.clip). The result is then the same as the result of func(np.asarray(da)) so units obviously get stripped. Should we make this work?

I marked all of these tests as xfail for now.

A few updates on the progress here:

The implementation of interpolate_na uses apply_ufunc with vectorize=True which in turn uses numpy.vectorize. numpy.vectorize is explicitly mentioned as deferred in NEP18 (i.e. not supported). I guess we can't support interpolate_na unless someone knows a way around that? Passing vectorize=False to apply_ufunc does make it work, though.

The failure of test_bivariate_ufunc is due to the way pint infers units, but whether or not that should matter was discussed in #3706 (but we didn't really reach a consensus there, I think?).

interp does work up until it calls scipy.interpolate.interp1d, which seems to strip the units?

reindex works for data, ~but the tests need a rewrite so they also test coords / dims with units (the same for the interp tests)~.

most of these are failing tests related to this PR but it is on hold until #3706 and #3611 (in that order) have been merged.

if you merge from master you should get all green on the upstream-dev test now

@jthielen, it looks like this needs numpy.pad and numpy.resize in pint. I tried to implement it myself, but while numpy.resize is easy (add an entry to the implement_consistent_units_by_argument loop), that is not the case for numpy.pad. Regardless of whether my proposed fixes are actually the best way, numpy.pad is used in other places of the code base so we'd probably need both, anyway. What do you think?

No problem at all to add these! See https://github.com/hgrecco/pint/pull/956.

@jthielen, it looks like this needs numpy.pad and numpy.resize in pint. I tried to implement it myself, but while numpy.resize is easy (add an entry to the implement_consistent_units_by_argument loop), that is not the case for numpy.pad. Regardless of whether my proposed fixes are actually the best way, numpy.pad is used in other places of the code base so we'd probably need both, anyway. What do you think?

~The aggregation tests fail because numpy.median needs __array_function__ to be implemented, but I think that's out of scope for this PR.~ I marked it as xfailing.

The bivariate ufunc test is fixed by hgrecco/pint#951.

shift could be fixed by using python filler = np.full_like(np.resize(trimmed_data, shape), fill_value, dtype=dtype) instead of https://github.com/pydata/xarray/blob/651f27fd5176674da315501026dc18a03b575a76/xarray/core/variable.py#L1105-L1112 but I didn't check if that would be a bad for performance (and pint would have to implement np.resize). However, that would not work for dask arrays so I'm not sure what to do.

rolling uses bottleneck which does not play nice with pint, yet. I'd suggest using python if bottleneck_move_func is not None and not isinstance( self.obj.data, dask_array_type ) and not hasattr(self.obj.data, "__array_function__"): instead of https://github.com/pydata/xarray/blob/651f27fd5176674da315501026dc18a03b575a76/xarray/core/rolling.py#L363-L365 If we go with that, we'd need pint to implement np.pad.

Edit: I added it, but this still requires np.pad. However, it makes a lot of other, unrelated tests fail, so I removed the commit again.

Unless I missed something, these should be the only issues left for pint support in DataArray (not counting the IndexVariable issue).

While trying to get the tests to pass, I encountered a few issues: * the implementation of __array_ufunc__: https://github.com/pydata/xarray/blob/3cbc459caa010f9b5042d3fa312b66c9b2b6c403/xarray/core/arithmetic.py#L37-L39 checks if it handles all types. However, the way it is implemented will cause it to return NotImplemented for duck arrays, even if it would be fine to treat them the same as ndarray. Adding and not hasattr(x, "__array_function__") will make my tests pass, but I thought I'd ask first. * I've been testing if the aggregation functions work, both as method and as function. This works fine for mean but not for median: calling np.median(da) returns a numpy scalar instead of the data array wrapping a 0d array returned by da.median(). I can't seem to find the reason why this happens, mostly because I don't know where to look.