Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NEP-18 Compatibility #905

Merged
merged 27 commits into from
Dec 11, 2019
Merged

NEP-18 Compatibility #905

merged 27 commits into from
Dec 11, 2019

Conversation

jthielen
Copy link
Contributor

@jthielen jthielen commented Nov 29, 2019

Building off of the implementation of __array_function__ in #764, this PR adds compatibility with NEP-18 in Pint (mostly in the sense of Quantity having __array_function__ and being wrappable as a duck array; for Quantity wrapping other duck arrays, see #845). Many tests are added of NumPy functions being used with Pint Quantities by way of __array_function__.

Accompanying changes that were needed as a part of this implementation include:

Closes #126
Closes #396
Closes #424
Closes #547
Closes #553
Closes #617
Closes #619
Closes #682
Closes #700
Closes #764
Closes #790
Closes #821

@keewis
Copy link
Contributor

keewis commented Nov 29, 2019

thank you for working on this, @jthielen. Don't worry about the slow progress, as long as it is not completely on hold.

Running the xarray tests results in quite a few errors (probably more if we count the xfails). I can't do much right now, but I will try to investigate and report more this weekend.

re inspect: I already mentioned it in #764, but I strongly support dropping py2 (not my call, though).
Also, I'd say let's move adding tests for wrapping other NEP-18-compatible arrays to another PR so we can keep this as small as possible.

Copy link
Contributor

@keewis keewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I glanced over the current state and found a few issues. I realize the code will change a lot in the future, but I will try to find the reason for the closure issue, anyway.

pint/quantity.py Outdated Show resolved Hide resolved
pint/quantity.py Outdated Show resolved Hide resolved
pint/quantity.py Outdated Show resolved Hide resolved
pint/quantity.py Outdated Show resolved Hide resolved
Copy link
Contributor

@keewis keewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this gets us down to 16 errors in test_numpy, most of them are missing implementations and offset unit problems. The only one left is the unwrap issue I mentioned. I tried to run the xarray test suite after disabling the xfail marks: there are a lot of issues, so let's first get the pint test suite to pass.

pint/quantity.py Outdated Show resolved Hide resolved
pint/quantity.py Outdated Show resolved Hide resolved
@jthielen
Copy link
Contributor Author

jthielen commented Dec 1, 2019

Thanks for the initial glace over the PR @keewis! In all my tinkering with this, I had forgotten to run the most recent set of changes past the test suite, which would have caught most of these. Whoops!

I've pushed some temporary fixups, which should get all the tests passing except the unimplemented ones. These changes may or may not be for naught after I redo the implementation logic using inspect.signature, which I hope to get to yet this weekend or early this coming week.

@keewis
Copy link
Contributor

keewis commented Dec 1, 2019

we should also keep in mind that since pint does not require numpy we need to make these changes optional: the tests without numpy installed currently fail because in that case np is None.

pint/testsuite/test_numpy.py Outdated Show resolved Hide resolved
@hgrecco hgrecco mentioned this pull request Dec 1, 2019
pint/compat/__init__.py Outdated Show resolved Hide resolved
pint/compat/__init__.py Outdated Show resolved Hide resolved
pint/compat/__init__.py Outdated Show resolved Hide resolved
@hgrecco
Copy link
Owner

hgrecco commented Dec 2, 2019

re inspect: I already mentioned it in #764, but I strongly support dropping py2 (not my call, though).

See #906 but in principle I agree

@hgrecco
Copy link
Owner

hgrecco commented Dec 3, 2019

As you sure have seen I have link a lot of issues to this PR. I am trying to make a list of numpy related issues that we should check again after this PR. Many of them will not be solved, but others might. The goal is to end 2019 with less open issues and PR!

@jthielen
Copy link
Contributor Author

jthielen commented Dec 4, 2019

I've finally reached the point where all tests are passing locally with NumPy available, so I think that means @keewis we can move forward with the next phase of testing with xarray!

Also, I've updated the initial comment with the main areas I need to work on before I'd feel ready to remove WIP/draft status.

bors bot added a commit that referenced this pull request Dec 11, 2019
905: NEP-18 Compatibility r=hgrecco a=jthielen

Building off of the implementation of `__array_function__` in #764, this PR adds compatibility with NEP-18 in Pint (mostly in the sense of Quantity having `__array_function__` and being wrappable as a duck array; for Quantity wrapping other duck arrays, see #845). Many tests are added of NumPy functions being used with Pint Quantities by way of `__array_function__`.

Accompanying changes that were needed as a part of this implementation include:
- a complete refactor of `__array_ufunc__` and ufunc attribute fallbacks to work in parallel with `__array_function__`
- promoting `_eq` in `quantity` to `eq` in `compat`
- preliminary handling of array-like compatibility by defining upcast types and attempting to wrap and defer to all others (a follow-up PR, or set of PRs, will be needed to completely address #845 / #878)

Closes #126 
Closes #396 
Closes #424
Closes #547 
Closes #553
Closes #617
Closes #619
Closes #682 
Closes #700
Closes #764
Closes #790
Closes #821

Co-authored-by: Jon Thielen <github@jont.cc>
@hgrecco
Copy link
Owner

hgrecco commented Dec 11, 2019

I have just asked bors to merge this PR. I would like to thank each and everyone who has contributed to this PR, either by code, bugs, ideas, documentation and testing. This is a major step forward in Pint and a great example of collaborative work and open source development.

Please review many Numpy related issues still open after this PR has been merged, closing those which are fixed and noting what is the new status after this PR.

@bors
Copy link
Contributor

bors bot commented Dec 11, 2019

Build succeeded

@jthielen
Copy link
Contributor Author

Please review many Numpy related issues still open after this PR has been merged, closing those which are fixed and noting what is the new status after this PR.

I've gone through all the linked issues that remained open and either commented on their status or incorporated them into more general issues such as #845 and #925, so hopefully that helps!

bors bot added a commit that referenced this pull request Dec 19, 2019
941: Fix infinite recursion with NumPy array/scalar raised to quantity power r=hgrecco a=jthielen

After running the current master branch against MetPy's test suite, I found that #905 introduced an infinite recursion error when the base of the exponent was a NumPy array or scalar by naively deferring to standard exponentiation. There were also issues when both the base and power were Quantities. This PR resolves those issues and adds basic tests for those cases.

- ~~Closes~~ (no associated issue)
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- ~~Documented in docs/ as appropriate~~
- ~~Added an entry to the CHANGES file~~ (fixup to previous change: #905)


Co-authored-by: Jon Thielen <github@jont.cc>
bors bot added a commit that referenced this pull request Dec 20, 2019
940: Reimplement cumulative product NumPy functions r=hgrecco a=jthielen

This PR resolves two issues from #905 that were identified in #939:

First, `convert_arg` in `numpy_func.py` presumes `pre_calc_units` is a `Unit` or `None`, but there are instances where it was passed a string by way of `inputs_units` in `implement_func`. Now, any `inputs_units` as a string are converted to a `Unit` before being passed to `convert_to_consistent_units`.

Second, the cumulative product functions were using `implement_func` with `input_units` set to something other than `None`, which was incorrect, since these functions have arguments that should not have units (namely `axis`). These functions have been reimplemented accordingly.

- [x] Closes #939 
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- ~~Documented in docs/ as appropriate~~
- ~~Added an entry to the CHANGES file~~ (fixup to previous change: #905)


Co-authored-by: Jon Thielen <github@jont.cc>
bors bot added a commit that referenced this pull request Dec 27, 2019
956: Add np.pad and np.resize implementations r=hgrecco a=jthielen

As requested in pydata/xarray#3643 (comment), this PR adds implementations for `np.pad` and `np.resize`, with accompanying tests.

- ~~Closes (insert issue number)~~ (#905 follow-up)
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- [x] Documented in docs/ as appropriate
- ~~Added an entry to the CHANGES file~~ (#905 follow-up)


Co-authored-by: Jon Thielen <github@jont.cc>
bors bot added a commit that referenced this pull request Dec 30, 2019
965: Raise ValueError on ambiguous boolean conversion and add pending NumPy functions/ufuncs from issue tracker r=hgrecco a=jthielen

There were a few follow-up issues from #905 that were missing fairly simple implementations, so this PR takes care of them in preparation for the upcoming release.

Part of this was resolving #866 by raising a `ValueError` due to ambiguity when casting Quantities with offset units to boolean, which is technically a breaking change (although I would argue that the previous behavior was incorrect as discussed in #866).

Also, @keewis, this implements `np.any` and `np.all`, which are two of the functions you had mentioned were needed by xarray. I think now the only one still missing is `np.prod` (#867) which will likely have to wait since there hasn't been a good solution yet.

- [x] Closes #419; Closes #470; Closes #807; Closes #866
- [x] Executed ``black -t py36 . && isort -rc . && flake8`` with no errors
- [x] The change is fully covered by automated unit tests
- [x] Documented in docs/ as appropriate
- [x] Added an entry to the CHANGES file


Co-authored-by: Jon Thielen <github@jont.cc>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment