Re: indexing improvements - advice wanted

[Jaroslav Hajek]

On Mon, Oct 27, 2008 at 7:36 PM, Jaroslav Hajek <address@hidden> wrote:
> On Mon, Oct 27, 2008 at 5:13 PM, David Bateman <address@hidden> wrote:
>> Jaroslav Hajek wrote:
>>>
>>> So, my questions (or opinion inquiries) are:
>>>
>>> 1. how much is backward compatibility of error messages desirable? In
>>> the current patch, I let Array<T>::resize () gripe when invalid
>>> resizing is requested (e.g. A(10) = 1 with A = ones(3)). However, the
>>> error message changes in such case; otherwise, an argument to resize
>>> would be needed to distinguish whether it's called from
>>> Array<T>::assign or from elsewhere. This is certainly possible, but is
>>> it worth cluttering interfaces by optional arguments just to keep
>>> error messages backward compatible? (note that they're by no means
>>> Matlab compatible, but that's not even reasonable to want)
>>
>> I don't think back compatiblity of error messages is that important. If
>> there is a good reason to change them, I'd say do it.
>>
>
> OK. The reason, as said above, is logic
>
>>
>>> 2. what about the failing tests relying on Matlab incompatible (and
>>> slightly illogical) behaviour? Should the tests be fixed, or should I
>>> try to adapt the code so that it behaves backward in a
>>> backward-compatible manner? Note that these are rather corner cases,
>>> quite unlikely to occur in real code.
>>
>> Less sure about this point. If the corner cases were added as test cases,
>> wwho were they added by? Maybe the person who added them should comment on
>> their need for these cases?
>>
>
> I think all tests are due to John. But I have yet to check closely
> which ones are bugs in my code. Eventually, I'll post any conflicting
> tests separately for discussion.
>
>>> and, of course, if anyone discovers the source of the concatenation
>>> bug, I'll be grateful :)
>>
>> Haven't looked at the patch so can't help out there (at least yet). In any
>> case I've always thought the indexing was needlessly slow and so this is a
>> big improvement and so I'm all for this patch.
>
> It's OK, I've already found the problem. Now I'm working to get the
> special cases of assigning into empty matrices work correctly.
>
>> Also it seems to me that a
>> case like
>>
>> A = speye(1e6,1e6)
>> A (A != 0) = randn(1e6,1);
>>
>
> Currently, I've only dealt with dense array indexing. I haven't yet
> looked at sparse arrays, so I'm not even sure I can achieve some
> optimizations there. In the current patch the only minor changes to
> the sparse code are those caused by the changed idx_vector interface.
> But I definitely intend to look at sparse matrices as well, at minimum
> I want to clean up the interfaces as I did with Array<T>.
>
>
>> that uses sparse logicall indexing would be easy to implement in your new
>> scheme as described, and this is one point that has always annoyed me.
>>
>
>
>> One point though, given that 3.2 is relatively close, perhaps this is
>> another patch to consider for 3.3 rather than 3.2 as with Michael's object
>> properties patch as the risk of obscure bugs is fairly high.
>>
>
> Agreed. Given that it's not yet finished, it's out of question, but in
> any case, I don't intend this to be applied before 3.2.x is forked.
>
>
>> Cheers
>> David
>>
>> --
>> David Bateman                                address@hidden
>> 35 rue Gambetta                              +33 1 46 04 02 18 (Home)
>> 92100 Boulogne-Billancourt FRANCE            +33 6 72 01 06 33 (Mob)
>>
>

The updated patch is uploaded, this time passing the test suite.
There were no flawed tests, after all - they were all bugs in my code.
So, the only changes in tests are caused by the different error
messages.
Of course, the presented speed-up numbers do still hold.

Unless anyone objects, I'd like to apply this after 3.2.x is forked.

cheers

-- 
RNDr. Jaroslav Hajek
computing expert
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz