MAINT: refactor PyArrayMultiIterObject constructors #13445
Conversation
There was a problem hiding this comment.
tl;dr: Makes the C-Api a bit more general, but it is a super corner case and seems OK, and not branching is much nicer.
It seems to me that the C-API functions PyArray_MultiIterNew and PyArray_MultiIterFromObjects previously only accepted arrays, while the Py-API also accepted iterator objects. The whole thing seems a bit strange to me, but on the other hand, it seems also harmless to generalize (and we could deprecate it at some point if we want to). It seem impossible that someone relies on MultiIterObjects getting broadcast when using the C-API. Those are two very very unlikely things together.
1957874 to
825b767
Compare
|
Updated with all requested changes, including using |
825b767 to
4f389dd
Compare
|
Forced pushed a hopefully good rebase (it was unfortunate I/we merged the other one first), undos some of the error message changes, but I do not think that matters. But it may need at least one more glance through now unfortunately. |
Creates a single private implementation of the PyArrayMultiIterObject constructor, and calls it from the three existing public constructors.
4f389dd to
17abad6
Compare
|
@seberg ping |
There was a problem hiding this comment.
Looks good to me, this undid the mnore detailed error messages now, but that higher detail did not give much anyway (negative number is implausible, so I think it is OK to not distinguish the two).
Cannot see anything wrong or any merge issue that arose. Matti, go ahead and merge just in case you wanted to have a glance because of the merge. Otherwise I will merge in a day or so.
Creates a single private implementation of the
PyArrayMultiIterObjectconstructor, and calls it from the three existing public constructors.
Rebased #7441 off master and made a new PR since I could not push the rebase to @jaimefrio 's repo. Of course we only need one of these, @jaimefrio if you wish to move forward on it feel free to close this one.