Skip to content

WIP: implement __rop__ logic for scalar operators - #7459

Closed
ewmoore wants to merge 3 commits into
numpy:masterfrom
ewmoore:scalar_math
Closed

ewmoore wants to merge 3 commits into
numpy:masterfrom
ewmoore:scalar_math

Conversation

@ewmoore

@ewmoore ewmoore commented Mar 24, 2016

Copy link
Copy Markdown
Contributor

Re. #7449.

This is a work in progress, both because I'd like to get some feedback on the approach and there are currently two test failures that I don't yet understand.

@ewmoore

ewmoore commented Mar 24, 2016

Copy link
Copy Markdown
Contributor Author

Also, negative integer powers still return an int type if one of the inputs is unsigned.

@charris charris added component: numpy._core 03 - Maintenance 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes labels Mar 24, 2016
@charris

charris commented Mar 24, 2016

Copy link
Copy Markdown
Member

I added a release notes tag as this will probably change behavior.

@ewmoore

ewmoore commented Mar 31, 2016

Copy link
Copy Markdown
Contributor Author

This is fun:

In [1]: import numpy as np

In [2]: np.__version__
Out[2]: '1.9.3'

In [3]: np.longdouble(3) * None
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-0319ac841cc8> in <module>()
----> 1 np.longdouble(3) * None

TypeError: unsupported operand type(s) for *: 'numpy.float128' and 'NoneType'

In [4]: None * np.longdouble(3)
/home/ewm/.pyenv/numpy-1.9.3-scipy-0.17.0-py27/bin/ipython:1: DeprecationWarning: using a non-integer number instead of an integer will result in an error in the future
  #!/home/ewm/.pyenv/numpy-1.9.3-scipy-0.17.0-py27/bin/python
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-4-9199aac90ce7> in <module>()
----> 1 None * np.longdouble(3)

RuntimeError: maximum recursion depth exceeded while calling a Python object

The fix for #1296 doesn't treat both arguments the same so this still fails. Working on a fix for this as well. The RunTimeError comes from linux. Under Windows this crashes the interpreter for me.

@ewmoore

ewmoore commented Apr 1, 2016

Copy link
Copy Markdown
Contributor Author

Alright. I think this is about ready to go.

@ewmoore

ewmoore commented Apr 1, 2016

Copy link
Copy Markdown
Contributor Author

This also results in a change to the outputs for true division of integer scalars. Now the usual integer to float promotion occurs so int8 / int16 -> float32. It used to go to float64 because, for reasons I don't understand integer array true_divide always goes to double.

@homu

homu commented Apr 7, 2016

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #6660) made this pull request unmergeable. Please resolve the merge conflicts.

@charris charris removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Apr 16, 2016
@charris

charris commented Apr 16, 2016

Copy link
Copy Markdown
Member

Needs rebase, probably the release notes.

@charris charris added this to the 1.12.0 release milestone Apr 16, 2016
ewmoore added 3 commits April 18, 2016 17:03
This is the second half of the fix for numpygh-1296 (trac 698).
None + np.longdouble(3) would cause either a RunTimeError or
an interpreter crash.  The swapped case (np.longdouble(3) + None)
was fixed in 2008 (376d483).

When writing binary operators in C, they must treat both arguments
equivalently because they will be called for both __op__ and
__rop__.  This bug fix is necessary because the longdouble and
clongdouble operators did not maintain this symmetry.
Previously it took 10x longer to do np.type1(1) op np.type2(2) when
type2 could not be safely cast to type1 than the equivalent operation
with type1 == type1 or when type2 could be safely upcast to type1. This
was due to falling back to calling the equivalent ufunc without trying
to defer the call to the scalar operator of type2 (and safely upcasting
type1)

so previously:

In [2]: a = np.float32(4)

In [3]: b = np.float64(4)

In [4]: timeit a * a
10000000 loops, best of 3: 69.1 ns per loop

In [5]: timeit b * b
10000000 loops, best of 3: 69.5 ns per loop

In [6]: timeit a * b
1000000 loops, best of 3: 1.29 µs per loop

In [7]: timeit b * a
10000000 loops, best of 3: 116 ns per loop

and with these changes:

In [2]: a = np.float32(4)

In [3]: b = np.float64(4)

In [4]: timeit a * a
10000000 loops, best of 3: 74 ns per loop

In [5]: timeit b * b
10000000 loops, best of 3: 73.7 ns per loop

In [6]: timeit a * b
10000000 loops, best of 3: 181 ns per loop

In [7]: timeit b * a
10000000 loops, best of 3: 125 ns per loop

Operations on mixed type scalars that result in a scalar of a new type
still use the ufunc fallback and hit the speed penalty e.g. F op d -> D.

In [2]: a = np.complex64(1+2j)

In [3]: b = np.double(4)

In [4]: timeit a * b
1000000 loops, best of 3: 1.22 µs per loop
When evaluating np.type1(2) op np.type2(4) don't fall back to calling
the op ufunc if the output type is neither of np.type1 or np.type2.
Defer the op call to that of the correct output type.

This speeds up things like F * d -> D by about 5x and with the previous
changes to the safely castable case, causes the scalar power operators
to always return a floating point type when raising integer types to
negative powers.

Fixes numpygh-7449.
@ewmoore

ewmoore commented Apr 18, 2016

Copy link
Copy Markdown
Contributor Author

Yep. Rebased.

@charris

charris commented Jun 4, 2016

Copy link
Copy Markdown
Member

@ewmoore It looks like the decision will be to raise a DeprecationWarning when raising scalar integers to negative integer powers except that zero division should raise an error. I think this PR should probably be split into two, one specifically for the negative exponent case, and the other for speeding things up. The first will be required for the 1.12.0 release.

@homu

homu commented Jun 4, 2016

Copy link
Copy Markdown
Contributor

☔ The latest upstream changes (presumably #7704) made this pull request unmergeable. Please resolve the merge conflicts.

@ewmoore

ewmoore commented Jun 6, 2016

Copy link
Copy Markdown
Contributor Author

@charris, I'm not going to be able to do much anytime soon. I've just started a new job and don't at present have a development machine.

@charris

charris commented Oct 7, 2016

Copy link
Copy Markdown
Member

Rebased in #8126. Thanks @ewmoore . How is the new job?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants