patch for structured array comparison bug
I found a bug in structured array comparison when fields have multi-dimensional types. I created a ticket here: http://projects.scipy.org/numpy/ticket/1640 and a patch here: http://github.com/m-paradox/numpy/compare/master...fix_structured_compare Could someone review it for me? Thanks, Mark
On Mon, Oct 18, 2010 at 7:13 PM, Mark Wiebe <mwwiebe@gmail.com> wrote:
I found a bug in structured array comparison when fields have multi-dimensional types. I created a ticket here:
http://projects.scipy.org/numpy/ticket/1640
and a patch here:
http://github.com/m-paradox/numpy/compare/master...fix_structured_compare
Could someone review it for me?
Just a quick look. I wasn't able to add comments to the code, maybe a pull request would allow that or maybe you need to enable something. Also, it is best to work in a branch, see the new notes<%20%20%20%20http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html>on the GIT workflow. I'm not sure how broadcasting is supposed to work for structured arrays so I will leave that to someone else. ISTR that the SUN compiler is persnickety about the initialization of structures, only accepts constants or some such. I'll try to track that down or maybe someone here who is familiar with that compiler can comment. Thanks for the work. Chuck
Tue, 19 Oct 2010 01:09:54 -0600, Charles R Harris wrote: [clip]
Just a quick look. I wasn't able to add comments to the code, maybe a pull request would allow that or maybe you need to enable something.
I think you can only add comments on commits, not in Github's compare view.
Also, it is best to work in a branch, see the new notes<%20%20%20%20http://docs.scipy.org/doc/numpy/dev/gitwash/ development_workflow.html> on the GIT workflow.
That work is in a separate branch, called `fix_structured_compare` :) But perhaps we should just recommend people filing pull requests right away?
I'm not sure how broadcasting is supposed to work for structured arrays so I will leave that to someone else. ISTR that the SUN compiler is persnickety about the initialization of structures, only accepts constants or some such. I'll try to track that down or maybe someone here who is familiar with that compiler can comment.
I suspect that this comparison code should refuse to compare arrays with shape(a) != shape(b), even if `a` and `b` are broadcastable to one another. The issue is that the broadcasting semantics work on the array level, but here the boolean sub-array is implicitly reduced to a single boolean. You can also initialize dimensions = shape(self) + (-1,) and let PyArray_Newshape do the size calculation for you. I guess it's best to not initialize structures directly, if there is some suspicion that obscure compilers don't like it. -- Pauli Virtanen
On Tue, Oct 19, 2010 at 2:24 AM, Pauli Virtanen <pav@iki.fi> wrote:
Tue, 19 Oct 2010 01:09:54 -0600, Charles R Harris wrote: [clip]
Just a quick look. I wasn't able to add comments to the code, maybe a pull request would allow that or maybe you need to enable something.
I think you can only add comments on commits, not in Github's compare view.
There are links to the commits at the top of the compare view, where the commenting appears to work. <snip>
But perhaps we should just recommend people filing pull requests right away?
Maybe describing one canonical way to do things (for newcomers, at least), would make things clear? Currently http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#basic-... <http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#basic-workflow>says "ask for a code review or make a pull request," so I chose the first option.
I'm not sure how broadcasting is supposed to work for
structured arrays so I will leave that to someone else. ISTR that the SUN compiler is persnickety about the initialization of structures, only accepts constants or some such. I'll try to track that down or maybe someone here who is familiar with that compiler can comment.
I suspect that this comparison code should refuse to compare arrays with shape(a) != shape(b), even if `a` and `b` are broadcastable to one another. The issue is that the broadcasting semantics work on the array level, but here the boolean sub-array is implicitly reduced to a single boolean.
I believe this would already be caught by the dtype comparison earlier in array_richcompare, here: http://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/arrayobj... You can also initialize
dimensions = shape(self) + (-1,)
and let PyArray_Newshape do the size calculation for you.
I guess it's best to not initialize structures directly, if there is some suspicion that obscure compilers don't like it.
I made both of these changes, visible here: http://github.com/m-paradox/numpy/compare/master...fix_structured_compare It looks like to make a pull request, I need to merge my branch back into master. Is that correct, or should I do a pull request from the branch? Either way, adding a note here about that would be helpful: http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#filing... -Mark
On Tue, Oct 19, 2010 at 1:00 PM, Mark Wiebe <mwwiebe@gmail.com> wrote:
On Tue, Oct 19, 2010 at 2:24 AM, Pauli Virtanen <pav@iki.fi> wrote:
Tue, 19 Oct 2010 01:09:54 -0600, Charles R Harris wrote: [clip]
Just a quick look. I wasn't able to add comments to the code, maybe a pull request would allow that or maybe you need to enable something.
I think you can only add comments on commits, not in Github's compare view.
There are links to the commits at the top of the compare view, where the commenting appears to work.
<snip>
But perhaps we should just recommend people filing pull requests right away?
Maybe describing one canonical way to do things (for newcomers, at least), would make things clear? Currently
http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#basic-...
<http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#basic-workflow>says "ask for a code review or make a pull request," so I chose the first option.
We're still working things out after the move, so you get to be sort of a guinea pig ;) I think a pull request is the way to go.
I'm not sure how broadcasting is supposed to work for
structured arrays so I will leave that to someone else. ISTR that the SUN compiler is persnickety about the initialization of structures, only accepts constants or some such. I'll try to track that down or maybe someone here who is familiar with that compiler can comment.
I suspect that this comparison code should refuse to compare arrays with shape(a) != shape(b), even if `a` and `b` are broadcastable to one another. The issue is that the broadcasting semantics work on the array level, but here the boolean sub-array is implicitly reduced to a single boolean.
I believe this would already be caught by the dtype comparison earlier in array_richcompare, here:
http://github.com/numpy/numpy/blob/master/numpy/core/src/multiarray/arrayobj...
You can also initialize
dimensions = shape(self) + (-1,)
and let PyArray_Newshape do the size calculation for you.
I guess it's best to not initialize structures directly, if there is some suspicion that obscure compilers don't like it.
I made both of these changes, visible here:
http://github.com/m-paradox/numpy/compare/master...fix_structured_compare
It looks like to make a pull request, I need to merge my branch back into master. Is that correct, or should I do a pull request from the branch? Either way, adding a note here about that would be helpful:
http://docs.scipy.org/doc/numpy/dev/gitwash/development_workflow.html#filing...
You can request a pull of the branch. Just give it a shot, you can always delete the request. You might want to rebase off the master branch first, however. Chuck
-
On Tue, Oct 19, 2010 at 12:35 PM, Charles R Harris < charlesr.harris@gmail.com> wrote:
<snip>
We're still working things out after the move, so you get to be sort of a guinea pig ;) I think a pull request is the way to go.
Sounds good. :)
<snip>
You can request a pull of the branch. Just give it a shot, you can always delete the request. You might want to rebase off the master branch first, however.
Done. It looked like I was already rebased off master, but I can try again if the pull request merges funny. -Mark
participants (3)
-
Charles R Harris -
Mark Wiebe -
Pauli Virtanen