Quantity.__array_ufunc__ #2583
Quantity.__array_ufunc__ #2583
Conversation
|
@mhvk - thanks! Should this be 1.0.0 rather than 0.4.0? I will try and review this soon. |
|
@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". |
|
@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 Is there a better benchmark we could use? |
|
@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: new: |
|
No I tested with 1.8 -- I should have read the PR to understand how much Numpy 1.9 matters... ;) |
|
@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) |
|
@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. |
|
@astrofrog - I realised that the tests I was writing were all for ufunc methods ( |
|
@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. |
|
@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. |
|
@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) |
Just to be sure I do not screw up, this is in |
|
@astrofrog - never mind, I just went ahead and tried. What could possibly go wrong... |
|
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 |
.travis.yml
Outdated
| @@ -55,6 +55,11 @@ matrix: | |||
| env: NUMPY_VERSION=1.6 SETUP_CMD='test' | |||
| - python: 2.7 | |||
| env: NUMPY_VERSION=1.5 SETUP_CMD='test' | |||
| # try bleeding edge numpy version | |||
There was a problem hiding this comment.
@mhvk - I think there is a space missing in front of #
There was a problem hiding this comment.
@mhvk -- actually, I think the problem is that you have a tab instead of spaces in front of the #.
There was a problem hiding this comment.
Turns out that when you view the file github automatically highlights tabs!
|
@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... |
|
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? |
|
I uploaded a new version that makes correct changes to get |
|
Now also the segfault issue has been reported to numpy, making it three bug reports based on this PR:
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! |
|
@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. |
|
@astrofrog - I'm trying to get back to implementing |
|
@mhvk - numpy 1.9 appears to now be in conda, so maybe the issue is moot :) |
026864e to
6907ecb
Compare
6907ecb to
0bca765
Compare
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.
0bca765 to
7a2b305
Compare
|
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.... |
astropy/units/quantity_helper.py
Outdated
| if unit is None: | ||
| raise TypeError("Cannot store non-quantity output{0} in {1} " | ||
| "instance".format( | ||
| ("from {0} function".format(function.__name__) |
astropy/units/quantity_helper.py
Outdated
| raise UnitTypeError( | ||
| "Cannot store output with unit '{0}'{1} " | ||
| "in {2} instance. Use {3} instance instead." | ||
| .format(unit, ("from {0} function".format(function.__name__) |
|
OK, fixed those. Since that's all untested strings, skipping CI and merging... |
|
After 3 years! 🎆 |
|
Indeed, it still needed a lot of work and to-and-fro on the numpy side.... |
|
Congratulations @mhvk and thank you so much for your persistence and numpy cross-project work! 🎉 |
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 previousxfails to be removed!). It also addresses #2483, so hopefully will help set us up for testing against numpy 1.9 in travis.Notes:
__numpy_ufunc__with methods other than__call__(e.g.,accumulate,reduce). Right now, I only allow two-argumentufuncs that do not change the unit (this is in the newscales_and_result_unitfunction, which is inquantity_helper). Do comment if this is wrong! Or if it is true for only some of those methods.