Conversation
There was a problem hiding this comment.
Also equivalent to the % operator, right?
There was a problem hiding this comment.
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.
I wonder if we should be using div, ldiv, and lldiv here?
There was a problem hiding this comment.
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.
Agreed, that is definitely clearer. Done.
numpy/lib/tests/test_mixins.py
Outdated
There was a problem hiding this comment.
You can use wrap_array_like here, right?
There was a problem hiding this comment.
Yes, but I think it's actually clearer to call ArrayLike() separately in cases where we know the arity of the arguments
There was a problem hiding this comment.
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.
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.
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.
It puzzles me that operator.pow is a thing, but operator.divmod is not
There was a problem hiding this comment.
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.
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.
Nit: resulting should probably be in both places or neither
There was a problem hiding this comment.
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.
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.
Only minor nitpicks from me - patch looks great!
numpy/core/tests/test_scalarmath.py
Outdated
There was a problem hiding this comment.
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.
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
doc/release/1.13.0-notes.rst
Outdated
There was a problem hiding this comment.
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.
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.
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.
I'd vote for just using remainder here.
There was a problem hiding this comment.
Yeah, no particular reason for this. Switched to use remainder.
There was a problem hiding this comment.
Can we add a reference from these functions back to divmod too?
There was a problem hiding this comment.
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.
Also note the reversed output values.
eric-wieser
left a comment
There was a problem hiding this comment.
Guessing you plan to rebase after a final review?
numpy/core/tests/test_umath.py
Outdated
There was a problem hiding this comment.
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.
Can modf link to divmod as well?
There was a problem hiding this comment.
Mention of the C library would be helpful: The C library ``modf`` function. Like ....
There was a problem hiding this comment.
Isn't this what the modf docstring is for? :)
There was a problem hiding this comment.
Not convinced the alignment of the colons buys anything here except extra spaces.
Yes, indeed |
There was a problem hiding this comment.
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.
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__?