Re: [OctDev] optimized lookup

[David Bateman]

Jaroslav Hajek wrote:
> I agree. My intention was to put this into Octave-Forge for anyone interested,
> so that anyone installing the Octave-Forge package gets the optimized
> version (I think .oct files have "higher priority" than .m files).
> If anyone of you Octave's maintainers decides that the function is worth
> including into Octave itself, you can simply do it (and possibly remove
> it from the Octave-Forge package).
>   
Looking at the speed improvements on the basis of the speed it makes
sense to include this code. However, speed is not the only basis to
consider. The next point is how often will the code in fact be used.
"lookup" is used by all of the interpolation functions for the "nearest"
and "linear" interpolation functions. Therefore, it is likely that it
will be used often. The next point to consider is the maintainability of
the code. The code is relatively short, so ok on that point as well.
However, it doesn't follow the coding style of the rest of Octave at
this point.

1) The copyright should be in /* .... */ comments
2) The Author field shoudl be below the copyright section
3) ret_idx_type should use octave_idx_type instead that is always the
indexing (32- or 64-bit) that is used
4) ret_idx_array type should in fact be an NDArray for consistency with
the original lookup function and so other Octave functions. Yes this
limits the indexing in a single dimension to 2^52 elements, but is that
really an issue.
5) The indentation style in Octave is more like

if (a == b)
  {
    /* Code here */
  }
else
  {
    ...
   }

rather than

if (a == b) {
  /* Code here */
} else {
  ...
}

that you've used. There are other coding style issues as well, but that
is the one that stuck out. If you can fix at least the ret_idx_type and
ret_idx_array issues, and John accepts the code, the other issues will
be easy enough to fix when the code is committed. John do you want this
function in the 3.1.x tree?

> Perhaps it would be a better idea to create a whole new package that
> would contain
> optimized (or otherwise improved) versions of Octave's library
> functions, so that they can
> be tested and eventually included.
>   
Only if we can't resolve rapidly whether this function should be
included in the 3.1.x tree of Octave or not.. In any case if it is
included in the 3.1.x tree, I believe we should keep octave-forge
targeted to Octave 3.0.x for the moment, so it might make even more
sense now that I think about it to have lookup.cc in octave-forge if it
is accepted for Octave 3.1.x..

D.




>
>   


-- 
David Bateman                                address@hidden
Motorola Labs - Paris                        +33 1 69 35 48 04 (Ph) 
Parc Les Algorithmes, Commune de St Aubin    +33 6 72 01 06 33 (Mob) 
91193 Gif-Sur-Yvette FRANCE                  +33 1 69 35 77 01 (Fax) 

The information contained in this communication has been classified as: 

[x] General Business Information 
[ ] Motorola Internal Use Only 
[ ] Motorola Confidential Proprietary