[Numpy-discussion] patch for structured array comparison bug

[Charles R Harris]

On Tue, Oct 19, 2010 at 1:00 PM, Mark Wiebe <mwwiebe at gmail.com> wrote:

> On Tue, Oct 19, 2010 at 2:24 AM, Pauli Virtanen <pav at 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-workflow
>
>
> <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/arrayobject.c#L932
>
> 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-a-pull-request
>
>
>
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

> -
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/numpy-discussion/attachments/20101019/c42d1211/attachment.html>