Quantity.__array_ufunc__ by mhvk · Pull Request #2583 · astropy/astropy

mhvk · 2014-05-30T20:53:39Z

EDIT (28-Apr-2017): in the end, the numpy implementation is called __array_ufunc__.

Following issue #2492, here is an implementation of __numpy_ufunc__ that at least locally passes all the tests (and allows some previous xfails to be removed!). It also addresses #2483, so hopefully will help set us up for testing against numpy 1.9 in travis.

Notes:

this requires With quantity output, ensure non-quantity input is treated as dimensionless #2571.
Rather rough is the implementation of __numpy_ufunc__ with methods other than __call__ (e.g., accumulate, reduce). Right now, I only allow two-argument ufuncs that do not change the unit (this is in the new scales_and_result_unit function, which is in quantity_helper). Do comment if this is wrong! Or if it is true for only some of those methods.
I let overrides of some methods depend on the numpy version; let me know if this is done the right way.

astrofrog · 2014-05-31T21:10:29Z

@mhvk - thanks! Should this be 1.0.0 rather than 0.4.0? I will try and review this soon.

mhvk · 2014-06-02T18:44:20Z

@astrofrog - I had not explicitly set a milestone -- as long as we do not care about numpy-1.9 for 0.4, I think we're fine. For now, I changed the milestone to "future".

mdboom · 2014-06-03T13:32:26Z

@mhvk: Since you had asked in a mailing list thread: This PR shows no significant performance change in any of the existing benchmarks. We have only one benchmark that really exercises ufuncs, though:

def time_quantity_ufunc_sin():
    np.sin(np.arange(10000) * u.degree)

Is there a better benchmark we could use?

mhvk · 2014-06-03T13:49:17Z

@mdboom - did you test with numpy-1.9? I get almost a factor two gain! (as expected if the sine calculation dominates and one removes one of the two calculations).

old:

In [1]: import astropy, astropy.units as u, numpy as np

In [2]: np.version.version, astropy.version.version
Out[2]: ('1.8.1', '0.4.dev8948')

In [3]: %timeit np.sin(np.arange(10000) * u.degree)
1000 loops, best of 3: 813 µs per loop

new:

In [1]: import astropy, astropy.units as u, numpy as np

In [2]: np.version.version, astropy.version.version
Out[2]: ('1.9.0.dev-9dd46ee', '0.4.dev8948')

In [3]: %timeit np.sin(np.arange(10000) * u.degree)
1000 loops, best of 3: 441 µs per loop

mdboom · 2014-06-03T14:31:45Z

No I tested with 1.8 -- I should have read the PR to understand how much Numpy 1.9 matters... ;)

astrofrog · 2014-06-13T11:34:09Z

@mhvk - could you check the test failures? It looks like some of them are legitimate? Given that Numpy 1.9 is close to release, I think we should at least make sure we have it ready in case we want to get it in 0.4 (and otherwise we can get it in 0.4.1)

mhvk · 2014-06-14T16:05:02Z

@astrofrog - I have been adding some extra tests, but am currently observing and will not get to this until later this week; think we'll have to aim for 0.4.1, at which time we may also be setting up travis to test with 1.9.

mhvk · 2014-06-16T21:03:29Z

@astrofrog - I realised that the tests I was writing were all for ufunc methods (np.add.reduce, etc.), which currently do not work anyway. So, I thought I'd better just try to get this PR to work as is; I think I've now done so, although there is a single failure that I do not understand. One issue: in order to merge this, we will need to have a numpy-1.9 test case on travis. Should this be set up already, or better wait for the official release? More generally, while it does seem to work for me, it would probably be good if you tested it as well...

astrofrog · 2014-06-17T07:05:34Z

@mwcraig - do you think it would be easy to create a binstar package for Numpy 1.9.0b1 for Python 2.6, 2.7, 3.1, 3.2, and 3.3? (http://sourceforge.net/projects/numpy/files/NumPy/1.9.0b1/). One the final Numpy 1.9 is out we can remove those, but it would be great to have them for testing in the mean time. If you don't have time, I can look into it later this week.

mwcraig · 2014-06-17T15:20:25Z

@astrofrog -- I think so...will report back later today either way. If I don't get to it today I won't get to it until the end of the week.

mwcraig · 2014-06-17T17:57:39Z

@astrofrog -- done and uploaded for: 2.6, 2.7, 3.2, 3.3, 3.4 (no 3.1 -- that isn't one of the pythons currently checked on travis)

astrofrog · 2014-06-17T19:00:04Z

@mwcraig - thanks!

@mhvk - can you add two entries for numpy 1.9.0b1 under the other numpy section in the travis file, one for python 2.7 and one for 3.4?

mhvk · 2014-06-17T20:36:43Z

@mhvk - can you add two entries for numpy 1.9.0b1 under the other numpy section in the travis file, one for python 2.7 and one for 3.4?

Just to be sure I do not screw up, this is in .travis.yml in the main
directory, correct?

mhvk · 2014-06-17T20:43:25Z

@astrofrog - never mind, I just went ahead and tried. What could possibly go wrong...

mhvk · 2014-06-17T20:56:39Z

Well, travis now immediately breaks, without an error message. I'm guessing this is because it cannot find the numpy version. Is it indeed called 1.9.0b1? (Don't know how to check...)

astrofrog · 2014-06-17T21:09:50Z

@mhvk - I think there is a space missing in front of #

@mhvk -- actually, I think the problem is that you have a tab instead of spaces in front of the #.

@mwcraig - good catch!

Turns out that when you view the file github automatically highlights tabs!

mhvk · 2014-06-17T23:50:45Z

@astrofrog, @mwcraig - thanks, that does seem to have done it! I've gotten so used to emacs converting tabs to spaces automatically that I had forgotten it depends on the mode it is in; clearly, it thinks that for .yml files tabs are OK...

astrofrog · 2014-06-18T07:46:33Z

Ugh, it looks like we are running into Numpy segfaults again. I thought @embray had fixed this upstream in Numpy (numpy/numpy#4494) and it looks like his fix did make it to 1.9.0b1, so this must be a new issue :(

@embray, since you've investigated this kind of issue before, any idea what is going on with the failures with Numpy 1.9.0b1? I am seeing those failures locally but I think not with the latest master, so I'm wondering whether something hasn't been backported?

mhvk · 2014-06-19T21:51:49Z

I uploaded a new version that makes correct changes to get Angle and its subclasses working, and a hack to avoid the segfaults; this really needs a bug report to numpy, but I wanted tests to be able to proceed (and EarthLocation will change anyway). There is still another issue in Angle related to subclasses not being dealt with quite right (see numpy/numpy#4815) and one in Time because __array_priority__ does not seem to be respected any more. Plus one in nddata that I haven't had time to investigate. It's all a bit more messy than hoped...

mhvk · 2014-07-09T11:07:57Z

Now also the segfault issue has been reported to numpy, making it three bug reports based on this PR:

subclass output behaviour differences: BUG difference in behaviour for subclass output in ufuncs numpy/numpy#4753
subclass TypeError: BUG __numpy_ufunc__ gives unexpected TypeError with subclasses numpy/numpy#4815
recarray comparison segfault: BUG subclass with __numpy_ufunc__ segfaults with recarray comparison numpy/numpy#4855

At the moment, the first two are also the two remaining blockers for 1.9, and I fear the third will be one too. I guess the positive side of this is that we are helping improve numpy as well!

astrofrog · 2014-07-09T14:41:43Z

@mhvk - thanks for identifying and reporting all these issues! On the plus side, if we weed out the bugs from 1.9, we won't need any version-specific workarounds.

mhvk · 2014-09-12T14:39:57Z

@astrofrog - I'm trying to get back to implementing __numpy_ufunc__, but to actually test it on travis, it would need to have the development version of numpy. Would it be possible in principle to set up travis to just download and compile numpy? (Just a pointer would be helpful! I tried to understand how the travis setup works, but fear I'm rather clueless.)

astrofrog · 2014-09-12T14:56:40Z

@mhvk - numpy 1.9 appears to now be in conda, so maybe the issue is moot :)

…Y<=1.7

Also clean up error types.

One was wrong in-place multiplication of an array with quantity, the other was that __array_ufunc__ got units of Parameters, but did not correctly apply these.

mhvk · 2017-06-17T22:26:53Z

Rebased, and fixed two regressions introduced by the modeling unit work -- a bug each in modeling and in my implementation. I think this is ready to go in....

adrn

I have two tiny nitpick comments, otherwise this is ready to merge when you add a changelog entry.

adrn · 2017-06-18T00:18:28Z

+        if unit is None:
+            raise TypeError("Cannot store non-quantity output{0} in {1} "
+                            "instance".format(
+                                ("from {0} function".format(function.__name__)


space before "from"?

adrn · 2017-06-18T00:18:42Z

+            raise UnitTypeError(
+                "Cannot store output with unit '{0}'{1} "
+                "in {2} instance.  Use {3} instance instead."
+                .format(unit, ("from {0} function".format(function.__name__)


again, space before "from"?

mhvk · 2017-06-18T13:19:38Z

OK, fixed those. Since that's all untested strings, skipping CI and merging...

pllim · 2017-06-19T15:17:44Z

After 3 years! 🎆

mhvk · 2017-06-19T15:36:58Z

Indeed, it still needed a lot of work and to-and-fro on the numpy side....

taldcroft · 2017-06-19T17:43:11Z

Congratulations @mhvk and thank you so much for your persistence and numpy cross-project work! 🎉

eteq added this to the v0.4.0 milestone May 30, 2014

mhvk modified the milestones: Future, v0.4.0 Jun 2, 2014

mhvk mentioned this pull request Jun 4, 2014

test failures with numpy-dev: elementwise comparison failed #2483

Closed

astrofrog reviewed Jun 17, 2014
View reviewed changes

mhvk mentioned this pull request Jun 19, 2014

BUG __numpy_ufunc__ gives unexpected TypeError with subclasses numpy/numpy#4815

Closed

mhvk mentioned this pull request Jul 10, 2014

Ensure in-place ops on Angles with non-angle result do not corrupt the instance #2718

Merged

embray force-pushed the master branch from cd9dfdf to 5d7acd6 Compare August 29, 2014 21:15

mhvk mentioned this pull request Jun 14, 2017

__numpy_ufunc__ confusion in docs numpy/numpy#9252

Open

mhvk force-pushed the quantity-numpy-ufunc branch from 026864e to 6907ecb Compare June 16, 2017 07:50

mhvk added 10 commits June 17, 2017 21:22

Introduce __numpy_ufunc__, adjust tests

84ed481

Ensure no None in arguments for result_type to avoid segfault on NUMP…

c692a54

…Y<=1.7

Avoid unnecessary upcasts.

4f22271

Change __numpy_ufunc__ to __array_ufunc__.

34aa1d7

Update following changes to __array_ufunc__ API.

9948348

Ensure ufunc.at, reduce, reduceat, accumulate, outer work

c50c3e8

Small fixups to documentation and tests.

815c285

Remove __pos__ for numpy >=1.13 (no longer needed).

ddce6f1

Rebase post-divmod, to accommodate two outputs better.

201c493

Safeguard against strange units in quantity_helper

1020db8

Also clean up error types.

mhvk force-pushed the quantity-numpy-ufunc branch from 6907ecb to 0bca765 Compare June 17, 2017 21:19

mhvk added the Ready-for-final-review label Jun 17, 2017

Fix bug that showed up with modeling becoming quantity-aware.

7a2b305

One was wrong in-place multiplication of an array with quantity, the other was that __array_ufunc__ got units of Parameters, but did not correctly apply these.

mhvk force-pushed the quantity-numpy-ufunc branch from 0bca765 to 7a2b305 Compare June 17, 2017 22:26

adrn approved these changes Jun 18, 2017

View reviewed changes

Small error string fixes; change-log entry [ci ckip]

cfc3852

mhvk added whatsnew-needed and removed Ready-for-final-review labels Jun 18, 2017

mhvk merged commit 5b18ebc into astropy:master Jun 18, 2017

mhvk deleted the quantity-numpy-ufunc branch June 18, 2017 13:19

bsipocz mentioned this pull request Jun 27, 2017

Transition Quantity to use __array_ufunc__ #2492

Closed

astrojuanlu mentioned this pull request Aug 11, 2017

Fix known test failures for astropy.units.quantity #1273

Closed

tien-vo mentioned this pull request Nov 3, 2024

Support for astropy.units pydata/xarray#9704

Closed

1 of 3 tasks

Conversation

mhvk commented May 30, 2014 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented May 31, 2014

Uh oh!

mhvk commented Jun 2, 2014

Uh oh!

mdboom commented Jun 3, 2014

Uh oh!

mhvk commented Jun 3, 2014

Uh oh!

mdboom commented Jun 3, 2014

Uh oh!

astrofrog commented Jun 13, 2014

Uh oh!

mhvk commented Jun 14, 2014

Uh oh!

mhvk commented Jun 16, 2014

Uh oh!

astrofrog commented Jun 17, 2014

Uh oh!

mwcraig commented Jun 17, 2014

Uh oh!

mwcraig commented Jun 17, 2014

Uh oh!

astrofrog commented Jun 17, 2014

Uh oh!

mhvk commented Jun 17, 2014

Uh oh!

mhvk commented Jun 17, 2014

Uh oh!

mhvk commented Jun 17, 2014

Uh oh!

astrofrog Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

mwcraig Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

astrofrog Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

mwcraig Jun 17, 2014

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 17, 2014

Uh oh!

astrofrog commented Jun 18, 2014

Uh oh!

mhvk commented Jun 19, 2014

Uh oh!

mhvk commented Jul 9, 2014

Uh oh!

astrofrog commented Jul 9, 2014

Uh oh!

mhvk commented Sep 12, 2014

Uh oh!

astrofrog commented Sep 12, 2014

Uh oh!

mhvk commented Jun 17, 2017

Uh oh!

adrn left a comment

Choose a reason for hiding this comment

Uh oh!

adrn Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

adrn Jun 18, 2017

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 18, 2017

Uh oh!

pllim commented Jun 19, 2017

Uh oh!

mhvk commented Jun 19, 2017

Uh oh!

taldcroft commented Jun 19, 2017

Uh oh!

Reviewers

mhvk commented May 30, 2014 •

edited

Loading