ENH: add np.divmod ufunc #9063

shoyer · 2017-05-06T01:58:25Z

TODO:

add it to NDArrayOperatorsMixin
update the ufunc overrides NEP
use it to implement ndarray.__divmod__?

eric-wieser · 2017-05-06T11:26:33Z

numpy/core/code_generators/ufunc_docstrings.py

+    See Also
+    --------
+    floor_divide : Equivalent of Python ``//`` operator.
+    remainder : Remainder complementary to floor_divide.


Also equivalent to the % operator, right?

Also add the equivalent for divmod(in, 1.):

modf : Return the fractional and integral parts of an array, element-wise.

eric-wieser · 2017-05-06T11:29:02Z

numpy/core/src/umath/loops.c.src

+        }
+        else {
+            /* handle mixed case the way Python does */
+            const @type@ rem = in1 % in2;


I wonder if we should be using div, ldiv, and lldiv here?

Scrap that, apparently it's better to leave the compiler to it. Might be sensible to have a const @type@ quo = in1 / in2; line right beside this one though, just to help it out

Agreed, that is definitely clearer. Done.

eric-wieser · 2017-05-06T12:38:54Z

numpy/lib/tests/test_mixins.py

-        check(np.frexp(ArrayLike(2 ** -3)))
-        check(np.frexp(ArrayLike(np.array(2 ** -3))))
+        mantissa, exponent = np.frexp(2 ** -3)
+        expected = (ArrayLike(mantissa), ArrayLike(exponent))


You can use wrap_array_like here, right?

Yes, but I think it's actually clearer to call ArrayLike() separately in cases where we know the arity of the arguments

mhvk

It looks good, but I remember that we had quite a few problems with ensuring floor_div and remainder were consistent with each other and with cpython (#7258); I suggest to add tests for divmod for all the test cases that were introduced in #7258.

mhvk · 2017-05-06T16:57:28Z

numpy/core/code_generators/ufunc_docstrings.py

+    See Also
+    --------
+    floor_divide : Equivalent of Python ``//`` operator.
+    remainder : Remainder complementary to floor_divide.


Also add the equivalent for divmod(in, 1.):

modf : Return the fractional and integral parts of an array, element-wise.

shoyer · 2017-05-06T19:12:21Z

I just did a quick benchmark, before switching the implementation of ndarray.__divmod__. Somewhat surprisingly, np.divmod is more than 2x faster in my micro-benchmark:

In [1]: import numpy as np

In [2]: x = np.arange(100000)

In [3]: %timeit np.floor_divide(x, 10)
1.33 ms ± 23.1 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [4]: %timeit np.remainder(x, 10)
1.35 ms ± 10.7 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [5]: %timeit divmod(x, 10)
2.7 ms ± 49.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [6]: %timeit np.divmod(x, 10)
1.19 ms ± 14 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

I'm not quite sure how that's possible, but there it is!

eric-wieser · 2017-05-06T19:17:54Z

That doesn't surprise me at all:

No python operator resolution to wait on
Only invoking the overhead of the ufunc machinery once, rather than twice
Looping over the inputs once rather than twice
Many processors calculate % and \ simultaneously, and then just give back the result that you ask for - so half as many ALU instructions here too (GCC can optimize once the div and mod are not in separate ufuncs).

eric-wieser · 2017-05-06T19:23:33Z

numpy/core/code_generators/ufunc_docstrings.py

+        Divisor array.
+    out : tuple of ndarray, optional
+        Arrays into which the output is placed. Their types are preserved and
+        must be of the right shape to hold the output. See doc.ufuncs.


Note that See doc.ufuncs doesn't actually produce a link of any kind, so should probably not be there at all, to avoid furthering the myth that it works.

Once we merge #9026, we'll get those links for every ufunc anyway.

shoyer · 2017-05-06T19:24:36Z

That doesn't surprise me at all:

Each of these would give you a 2x speedup, but usually there's also some small fixed overhead. I would expect the time required would go from fixed_setup + 2 * compute_divmod to fixed_setup + compute_divmod. Here it looks like the fixed overhead is negative?!?

mhvk · 2017-05-06T19:28:14Z

@shoyer - floor_div and remainder both effectively do a full divmod to ensure the results are correct -- see #7258 -- so a factor two improvement is in fact expected!

shoyer · 2017-05-06T19:29:13Z

@mhvk yes, but how do you account for 2.27x speedup? :)

eric-wieser · 2017-05-06T19:29:50Z

Here it looks like the fixed overhead is negative?!?

Only that the fixed overhead is smaller in the case of np.divmod - which is what I would expect, since you removed the overhead of PyNumber_DivMod in that test

eric-wieser · 2017-05-06T19:35:09Z

numpy/core/code_generators/ufunc_docstrings.py


    Returns
    -------
    out1 : ndarray
-        Element-wise result of floor division.
+        Element-wise quotient resulting from floor division.
    out2 : ndarray
        Element-wise remainder from division.


If you're mentioning floor division above, then presumably you should do so here too?

shoyer · 2017-05-06T19:43:48Z

I still don't get how np.divmod is faster than calling np.floor_divide or np.remainder and throwing away half the result, but I don't need to understand it to appreciate it :).

I'm about to add this as the implementation forndarray.__divmod__ and add the remaining test cases from #7258.

One question for the bikeshedders: what do we call this function? NumPy ufuncs have full non-abbreviated names, e.g., np.divide and np.remainder for the two separated operators. This suggests several possible alternatives to np.divmod:

np.divmod: consistent with Python's name for the operation, but masks the builtin divmod if write from numpy import *.
np.divide_remainder: consistent with the equivalent ufuncs that return one result, but one name is a verb and the other is a noun, which is an awkward combination. Unfortunately, there doesn't seem to be a single verb for "to take the remainder".
np.division_modulo: consistent with the Python's operator names
np.quotient_remainder: Both nouns.

eric-wieser · 2017-05-06T19:47:04Z

NumPy ufuncs have full non-abbreviated names,

This actually catches me out a lot (especially np.mul not being a thing), so I'd favor sticking with divmod to minimize that pain. np.abs, np.all, and np.any already result in name-hiding, so I don't think that needs to be a concern.

eric-wieser · 2017-05-06T19:50:08Z

numpy/lib/tests/test_mixins.py

+    # TODO: test div on Python 2, only
+    operator.mod,
+    divmod,
+    pow,


It puzzles me that operator.pow is a thing, but operator.divmod is not

All these are operators in the sense that they have a corresponding symbol. (Hopefully, we'll have a operator.matmul in here shortly...)

Good catch. In particular, operator.pow has two arguments, but pow has three

shoyer · 2017-05-06T22:36:45Z

Please take another look. I think I've finished up the changes I wanted to include here.

(Fixing the docstring for np.isin was required to get the doc build to run)

eric-wieser · 2017-05-06T23:07:11Z

numpy/core/code_generators/ufunc_docstrings.py

+    Returns
+    -------
+    out1 : ndarray
+        Element-wise quotient resulting from floor division.


Nit: resulting should probably be in both places or neither

The quotient is the result of floor division, but the remainder is a by-product. So I think the current language makes sense (but I'm open to alternatives if you have a concrete suggestion).

I'm not convinced results and byproducts are disjoint sets, but I'm happy to leave this as is based on your rationale.

eric-wieser

Only minor nitpicks from me - patch looks great!

eric-wieser · 2017-05-06T23:10:52Z

numpy/core/tests/test_scalarmath.py

-                    assert_(b < rem <= 0, msg)
-                else:
-                    assert_(b > rem >= 0, msg)
+        for op in [floordiv_and_mod, divmod]:


Perhaps before this loop, or even at the file level:

signs = { d: (+1,) if dt in np.typecodes['UnsignedInteger'] else (+1, -1) for d in dt }

And then itertools.product(signs[dt1], signs[dt2]) below, which removes the continues

Actually, better would be, in the class level:

def _signs(self, dt): if dt in np.typecodes['UnsignedInteger']: return (+1,) else: return (+1, -1)

Constructing that dict is kinda clunky, and you'd end up repeating it over multiple tests

eric-wieser · 2017-05-06T23:14:10Z

doc/release/1.13.0-notes.rst

+This ufunc corresponds to the Python builtin `divmod`, and is used to implement
+`divmod` when called on numpy arrays. ``np.divmod(x, y)`` calculates a result
+equivalent to ``(np.floor_divide(x, y), np.remainder(x, y))`` but is
+approximately twice as fast as calling the functions separately.


~~Probably worth pointing out that the builtin divmod now dispatches to this~~ - I'm an idiot, you've already done this

And before release I'm going to gather all the new ufuncs into a New ufuncs section. There are enough of them in the 1.13 release to justify that.

mhvk · 2017-05-07T15:00:32Z

@shoyer - this looks good! I like the loops over divmod and (floor_div, remainder) -- a very direct way to ensure they are identical to each other! Only the few nitpicks by @eric-wieser left, I think.

eric-wieser · 2017-05-07T15:04:25Z

doc/neps/ufunc-overrides.rst

@@ -663,14 +663,14 @@ Symbol Operator     NumPy Ufunc(s)
       (Python 2)
 ``//`` ``floordiv`` :func:`floor_divide`
 ``%``  ``mod``      :func:`mod`


Is there a reason we use mod here and not remainder? If mod is preferable, then should we reverse the alias, so that np.mod.__name__ == 'mod'?

I'd vote for just using remainder here.

Yeah, no particular reason for this. Switched to use remainder.

eric-wieser · 2017-05-07T15:05:59Z

numpy/core/code_generators/ufunc_docstrings.py

+    --------
+    floor_divide : Equivalent of Python ``//`` operator.
+    remainder : Equivalent of Python ``%`` operator.
+    modf : Equivalent to ``divmod(x, 1.0)``.


Can we add a reference from these functions back to divmod too?

The modf function comes from the C library and doesn't agree with the Python divmod for negative floats.

In [1]: np.modf(-1.5) Out[1]: (-0.5, -1.0) In [2]: divmod(-1.5, 1) Out[2]: (-2.0, 0.5)

Also note the reversed output values.

eric-wieser

Guessing you plan to rebase after a final review?

eric-wieser · 2017-05-07T19:29:42Z

numpy/core/tests/test_umath.py

+    if dt in np.typecodes['UnsignedInteger']:
+        return (+1,)
+    else:
+        return (+1, -1)


Darn, I'd missed that this was in two different files, which makes extracting it a little less useful. I guess this is still a minor improvement though

eric-wieser · 2017-05-07T19:29:45Z

numpy/core/code_generators/ufunc_docstrings.py

+    remainder    : Equivalent to Python's ``%`` operator.
+    modf         : Like ``divmod(x, 1.0)`` for positive ``x``, but returns
+                   ``(remainder, quotient)`` instead of
+                   ``(quotient, remainder)``.


Can modf link to divmod as well?

Mention of the C library would be helpful: The C library ``modf`` function. Like ....

Isn't this what the modf docstring is for? :)

Not convinced the alignment of the colons buys anything here except extra spaces.

shoyer · 2017-05-07T20:38:51Z

Guessing you plan to rebase after a final review?

Yes, indeed

eric-wieser · 2017-05-07T20:40:11Z

numpy/core/code_generators/ufunc_docstrings.py

@@ -2474,6 +2475,10 @@ def add_newdoc(place, name, doc):
    -----
    For integer input the return values are floats.

+    See Also
+    --------
+    divmod : Simultaneous floor division and remainder.


I was envisaging likening divmod(x, 1) to modf here, in the same way we do the reverse in divmod. If anything, this direction is more important to get the message across, since most users probably want the behaviour of divmod on negative values

eric-wieser · 2017-05-07T23:14:43Z

LGTM. Ready for a final rebase, I think

eric-wieser · 2017-05-07T23:15:25Z

numpy/core/code_generators/ufunc_docstrings.py

+    Examples
+    --------
+    >>> np.divmod(np.arange(5), 3)
+    array([0, 0, 0, 1, 1]), array([0, 1, 2, 0, 1])


Spoke too soon - missing parentheses here

shoyer · 2017-05-08T00:06:00Z

OK, git history has been rationalized. Feel free to merge once tests pass.

eric-wieser · 2017-05-08T00:24:42Z

Thanks @shoyer!

shoyer mentioned this pull request May 6, 2017

Missing ufuncs: np.divmod and np.positive #8932

Closed

eric-wieser reviewed May 6, 2017

View reviewed changes

charris added 01 - Enhancement component: numpy._core component: numpy.lib labels May 6, 2017

mhvk reviewed May 6, 2017

View reviewed changes

eric-wieser reviewed May 6, 2017

View reviewed changes

shoyer added this to the 1.13.0 release milestone May 6, 2017

shoyer force-pushed the divmod branch from 396c56e to 745a6c7 Compare May 6, 2017 22:36

eric-wieser reviewed May 6, 2017

View reviewed changes

eric-wieser approved these changes May 6, 2017

View reviewed changes

eric-wieser reviewed May 7, 2017

View reviewed changes

eric-wieser mentioned this pull request May 7, 2017

ENH: Show full PEP 457 argument lists for ufuncs #9026

Merged

eric-wieser reviewed May 7, 2017

View reviewed changes

shoyer added 5 commits May 7, 2017 17:03

ENH: add np.divmod ufunc

c9d1f9e

ENH: add divmod support to NDArrayOperatorsMixin

d51b538

ENH: switch ndarray.__divmod__ to use np.divmod

6144637

DOC: update ufunc overides NEP with __divmod__

a148978

DOC: fix docstring for np.isin

8fbf75e

shoyer force-pushed the divmod branch from 7d20939 to 8fbf75e Compare May 8, 2017 00:04

eric-wieser merged commit 11f3ebf into numpy:master May 8, 2017

homu mentioned this pull request May 8, 2017

ENH: Add gcd and lcm ufuncs #8774

Merged

shoyer deleted the divmod branch May 8, 2017 00:58

ENH: add np.divmod ufunc #9063

ENH: add np.divmod ufunc #9063

Conversation

shoyer commented May 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented May 6, 2017

eric-wieser commented May 6, 2017

Choose a reason for hiding this comment

shoyer commented May 6, 2017

mhvk commented May 6, 2017

shoyer commented May 6, 2017

eric-wieser commented May 6, 2017 • edited

Choose a reason for hiding this comment

shoyer commented May 6, 2017

eric-wieser commented May 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser May 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser May 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris May 7, 2017 • edited

Choose a reason for hiding this comment

shoyer commented May 7, 2017

eric-wieser May 7, 2017 • edited

Choose a reason for hiding this comment

eric-wieser commented May 7, 2017

Choose a reason for hiding this comment

shoyer commented May 8, 2017

eric-wieser commented May 8, 2017

shoyer commented May 6, 2017 •

edited

eric-wieser commented May 6, 2017 •

edited

eric-wieser commented May 6, 2017 •

edited

eric-wieser May 6, 2017 •

edited

eric-wieser May 6, 2017 •

edited

charris May 7, 2017 •

edited

eric-wieser May 7, 2017 •

edited