Re: indexing improvements - advice wanted

[Jaroslav Hajek]

On Wed, Oct 29, 2008 at 6:54 PM, John W. Eaton <address@hidden> wrote:
> On 28-Oct-2008, Jaroslav Hajek wrote:
>
> | 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.
>
> I don't care about error messages changing from one version to the
> next.
>
> I have some more things to try, but so far these changes seem to be
> producing the same results as the older code.
>

If you're trying more corner cases, perhaps you could add them to the
test suite? It was a big help when I was doing this.

> | Of course, the presented speed-up numbers do still hold.
>
> Those are impressive speedups, and the code looks much cleaner.
>
> | Unless anyone objects, I'd like to apply this after 3.2.x is forked.
>
> Is there any objection to applying it now?  I don't expect 3.2 will be
> released before making a number of snapshots, and we would likely get
> feedback about any indexing problems before the release.  From the
> looks of things, it would probably be easier to fix any indexing bugs
> in the new code than it was in the old.
>

I'm not sure - I guess it depends on how far off 3.2.0 is. I wanted to
also look at Sparse<T> (see my previous reply to David), so this patch
has only gone "halfway", and if 3.2.0 is released, e.g., next week, I
may not make it to finish the rest. But I really can't tell whether
this is enough of a reason.

> You have the following comments in the Array<T>::index (const
> Array<idx_vector>& ia) const function:
>
>  // FIXME: is this dispatching necessary?
>  if (ial == 1)
>    retval = index (ia(0));
>  else if (ial == 2)
>    retval = index (ia(0), ia(1));
>
> and a similar one in the Array<T>::assign (const Array<idx_vector>&
> ia, const Array<T>& rhs, const T& rfv) function:
>
>  // FIXME: is this dispatching necessary / desirable?
>  if (ial == 1)
>    assign (ia(0), rhs, rfv);
>  else if (ial == 2)
>    assign (ia(0), ia(1), rhs, rfv);
>  else if (ial > 0)
>
> What is the speed comparison for one- and two-index expressions when
> handled with the generic N-index function?  Unless there is a major
> slowdown, I would be in favor of having only one function to do the
> actual indexing, though we could preserve specialized one- and
> two-index functions for backward compatibility.
>

The single-subscript specialization are necessary, because single
subscripts behave differently enough. I created two-subscripts
specializations for several reasons:
1. It is the most common case.
2. The old code did so.
3. They avoid the rec_index_helper class machinery and recursive calls
in favor of a single index reduction and a simple loop, so I hoped it
would save bits of performance.

I don't think any of them is particularly strong, but together, they
justified the thing for me. I guess the performance advantage will be
barely visible, but I can test it.
Simply comparing speed of a(i,j) vs. a(i,j,1) should actually do the trick.

> Maybe we should also introduce some functions like
>
>  Array<T>
>  Array<T>::operator () (const idx_vector& i) const;
>
>  Array<T>
>  Array<T>::operator () (const idx_vector& i, const idx_vector& j) const;
>
>  ...
>
> so that we can write things like
>
>  Array<T> ra1 (...);
>  octave_idx_type i, j;
>  ...
>  Array<T> ra2 = ra1(i,j);
>
> ?
>
> jwe
>

Good idea. I guess we can simply overload the () operators to do that.
I'm not sure, however, whether an analogical thing can easily be
achieved to support indexed assignment (we would need to return some
kind of indexed proxy).
If not, then maybe it is not such a good idea, because if b = a(i)
works, people will IMHO tend to assume that a(i) = b works as well.


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