Re: indexing improvements - advice wanted

[Jaroslav Hajek]

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)
>



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