Re: issorted & sortrows

[Jaroslav Hajek]

On Wed, Feb 11, 2009 at 4:02 PM, John W. Eaton <address@hidden> wrote:
> On 11-Feb-2009, Jaroslav Hajek wrote:
>
> | following the recent optimization of sort, I went ahead and
> | implemented issorted and optimized sortrows:
>
> Your patch has
>
>  diff --git a/liboctave/Array-C.cc b/liboctave/Array-C.cc
>  --- a/liboctave/Array-C.cc
>  +++ b/liboctave/Array-C.cc
>  @@ -28,41 +28,79 @@
>   // Instantiate Arrays of Complex values.
>
>   #include "oct-cmplx.h"
>  +#include "lo-mappers.h"
>
>   #include "Array.h"
>   #include "Array.cc"
>   #include "oct-sort.cc"
>
>  -static double
>  -xabs (const Complex& x)
>  +template <>
>  +inline bool _sort_isnan (Complex x)
>   {
>  -  return (xisinf (x.real ()) || xisinf (x.imag ())) ? octave_Inf : abs (x);
>  +  return xisnan (x);
>   }
>
> Why the change from "const Complex&" to "Complex"?  That means you are
> copying the Complex data structure on each call.  I think this should
> still be a call by const reference.
>

It's not really a change, because this is a completely different
function. Since it is marked inline and very simple, I think it will
be always inlined, so the speed should be the same. In fact
_sort_isnan is a purely convenience wrapper just to make the code in
Array.cc more generic. It will always either return constant true (if
a type has no NaN) or forward the call.

> For Octave, please also use "x" as a prefix for functions like this
> instead of "_".

I thought x was the prefix for mappers; this is something different.
But feel free to adjust the style wherever you wish.

>   template <>
>   bool
>   octave_sort<Complex>::ascending_compare (Complex a, Complex b)
>   {
>  -  return (xisnan (b) || (xabs (a) < xabs (b))
>  -       || ((xabs (a) == xabs (b)) && (arg (a) < arg (b))));
>  +  return ((std::abs (a) < std::abs (b))
>  +       || ((std::abs (a) == std::abs (b)) && (arg (a) < arg (b))));
>   }
>
> Apparently this was already call by value?  I think these arguments
> should also be "const Complex&", not "Complex".
>

Here, this is not so easily possible. The type of
octave_sort<T>::compare is bool (*) (T, T), not bool (*) (const T&,
const T&), so you'd get a type mismatch. The first form is probably
more suitable for simple built-in types, the latter for complex ones.
Supporting both would be possible but somewhat complicated.

>  +static bool nan_ascending_compare (Complex x, Complex y)
>  +{
>  +  return xisnan (y) ? ! xisnan (x) : ((std::abs (x) < std::abs (x))
>  +       || ((std::abs (x) == std::abs (x)) && (arg (x) < arg (x))));
>  +}
>
> For consistency with the rest of the Octave sources, and so that
>
>  grep ^FCN_NAME
>
> will find function definitions (but not uses), please write
>
>  static bool
>  nan_ascending_compare (const Complex& x, const Complex& y)
>

OK, sorry. I usually do this; however, this is again just a helper
function (that's why it's static).

>
> +bool (*_sortrows_comparator (sortmode mode,
> +                             const Array<Complex>& a , bool allow_chk))
> +(Complex, Complex)
> +{
>
> This has me scratching my head.  Maybe a typedef would help here, so
> you could write this as
>
>  sortrows_comparator_function
>  xsortrows_comparator (const Complex&, const Complex&)
>  {
>

Here, I simply copied the template from Array.cc

template<class T>
bool (*_sortrows_comparator (...))(T, T) { ... }

I don't think this syntax can be simplified without introducing a
traits class. For the specializations, one can introduce explicit
typedefs (like you have shown) but it seems to me that it obscures
somewhat the fact that it's a specialization (matching signatures).

> ?  Or am I completely missing the point of this function signature?
> If so, then I think a comment might help here, as this looks a little
> obscure and is sure to confuse more people than just me.
>

OK, a comment would help. It's just after working at octave_sort for a
few days it seemed natural to me :)

cheers


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