ENH: ufuncs can take a tuple of arrays as 'out' kwarg #5621
Conversation
|
As discussed in #4752 this is currently failing the tests, because these expect ufuncs with multiple returns to accept a single array in the Old behavior
New behavior
Changes to maintain backwards compatibility
|
b876c2d to
019360e
Compare
|
OK, this seems to be working now. I still need to document the new behavior, and since I also had to make changes to the array wrapping might be a good idea to add some tests for that. But I'd appreciate a first review. |
| } | ||
| else if (nout == 1) { | ||
| /* Can be an array if it only has one output */ | ||
| if (_check_out_array(value, out_op+nin) < 0) { |
There was a problem hiding this comment.
Overall, to the extent I can judge, this looks good and clearer than the code that was present. A perhaps nitpick is that I think assignments inside if functions make the code less clear than it could be otherwise. I would suggest replacing this stanza (and similar ones) with something like the following:
array_ok = _set_out_array(value, out_op+nin)
if (array_ok < 0) {
...
Here, I also changed the name to ensure it is clear what the function does; it could also be _check_and_set_out_array or _set_out_array_if_not_None or so).
There was a problem hiding this comment.
Good suggestion on the name of the function, will change. But single function calls inside an if statement, when the return isn't needed elsewhere, are everywhere in the numpy code base, and in general in all Python C extensions, starting with the if (!PyArg_ParseTupleAndKeywords...) {return NULL;} in the python.org tutorial on C extensions.
I really think that adding a variable is unnecessary bulk in this case, so am going to stand my ground on that one, unless there is an overwhelming opinion against.
There was a problem hiding this comment.
Not being used very much to C I'm happy to follow convention -- though this particular piece of code did not seem to follow it originally. Anyway, I think that with the name change it will be much more obvious that the main action is something being set, not just checked.
019360e to
590900a
Compare
|
I have incorporated some of Marten's suggestions, and added some more tests to validate the wrapping behavior. Documenting the new behavior is the only thing left, but the code itself should be almost ready to go. |
590900a to
cb61691
Compare
|
I have made some changes, to properly report the error when the tuple does not have as many items as output arguments. I have also added some testing for the proper errors being raised. I think now only the documentation part is left. |
cb61691 to
f931459
Compare
|
Ok, have added the documentation changes too. I think it is ready to go. |
f931459 to
c82cf07
Compare
|
|
||
| ..versionadded:: 1.10 | ||
|
|
||
| The 'out' keyword argument is now expected to be a tuple with one entry |
There was a problem hiding this comment.
I'd delete "now" here. Documentation should aim at those without previous knowledge; the versionadded will be enough for those who did know.
|
Looks good! Only two very minor comments. |
c82cf07 to
854a974
Compare
|
Minor but sensible, should be corrected in the latest push. |
| PyErr_SetString(PyExc_TypeError, | ||
| "return arrays must be " | ||
| "of ArrayType"); | ||
| if (_set_out_array(obj, out_op+i) < 0) { |
|
Few style nitpicks, otherwise LGTM. Does this affect #5566? |
854a974 to
1a7699f
Compare
1a7699f to
bb3ae05
Compare
ENH: ufuncs can take a tuple of arrays as 'out' kwarg
|
Thanks Jaime. Noticed a few more places needing spaces, but I think we should make a script to find those and do a general cleanup at some point. |
|
thanks this is great, I think using tuple makes much more sense, makes me wonder why was this not originally done like this. though now: x = np.arange(9)
o1 = np.empty(9)
o2 = np.empty(9, dtype=np.int32)
np.frexp(x, out=o1, out2=o2)
ValueError: cannot specify 'out' as both a positional and keyword argument |
|
Should we be more strict in the checking of |
|
Should we be more strict in the checking of 'out' being just 'out'?
+10
|
|
I'm not sure why the code uses |
|
I guess it's either an ancient thinko, or else maybe someone was planning
|
|
👍 to checking |
|
Take a look at #5659 . |
Closes #4752