-
-
Notifications
You must be signed in to change notification settings - Fork 11.9k
ENH: add np.divmod ufunc #9063
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
ENH: add np.divmod ufunc #9063
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also equivalent to the % operator, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add the equivalent for divmod(in, 1.):
modf : Return the fractional and integral parts of an array, element-wise.
numpy/core/src/umath/loops.c.src
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be using div, ldiv, and lldiv here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that is definitely clearer. Done.
numpy/lib/tests/test_mixins.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use wrap_array_like here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think it's actually clearer to call ArrayLike() separately in cases where we know the arity of the arguments
mhvk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add the equivalent for divmod(in, 1.):
modf : Return the fractional and integral parts of an array, element-wise.
|
I just did a quick benchmark, before switching the implementation of I'm not quite sure how that's possible, but there it is! |
|
That doesn't surprise me at all:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
|
@mhvk yes, but how do you account for 2.27x speedup? :) |
Only that the fixed overhead is smaller in the case of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're mentioning floor division above, then presumably you should do so here too?
|
I still don't get how I'm about to add this as the implementation for One question for the bikeshedders: what do we call this function? NumPy ufuncs have full non-abbreviated names, e.g.,
|
This actually catches me out a lot (especially |
numpy/lib/tests/test_mixins.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It puzzles me that operator.pow is a thing, but operator.divmod is not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these are operators in the sense that they have a corresponding symbol. (Hopefully, we'll have a operator.matmul in here shortly...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. In particular, operator.pow has two arguments, but pow has three
|
Please take another look. I think I've finished up the changes I wanted to include here. (Fixing the docstring for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: resulting should probably be in both places or neither
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor nitpicks from me - patch looks great!
numpy/core/tests/test_scalarmath.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
doc/release/1.13.0-notes.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth pointing out that the builtin - I'm an idiot, you've already done thisdivmod now dispatches to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
@shoyer - this looks good! I like the loops over divmod and |
doc/neps/ufunc-overrides.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for just using remainder here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no particular reason for this. Switched to use remainder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a reference from these functions back to divmod too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also note the reversed output values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
eric-wieser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guessing you plan to rebase after a final review?
numpy/core/tests/test_umath.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can modf link to divmod as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention of the C library would be helpful: The C library ``modf`` function. Like ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this what the modf docstring is for? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not convinced the alignment of the colons buys anything here except extra spaces.
Yes, indeed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
LGTM. Ready for a final rebase, I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke too soon - missing parentheses here
|
OK, git history has been rationalized. Feel free to merge once tests pass. |
|
Thanks @shoyer! |
Fixes #8932
TODO:
ndarray.__divmod__?