[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: issorted & sortrows
From: |
Jaroslav Hajek |
Subject: |
Re: issorted & sortrows |
Date: |
Wed, 11 Feb 2009 19:14:43 +0100 |
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
- issorted & sortrows, Jaroslav Hajek, 2009/02/11
- issorted & sortrows, John W. Eaton, 2009/02/11
- Re: issorted & sortrows,
Jaroslav Hajek <=
- Re: issorted & sortrows, John W. Eaton, 2009/02/11
- Re: issorted & sortrows, Jaroslav Hajek, 2009/02/11
- Re: issorted & sortrows, David Bateman, 2009/02/11
- Re: issorted & sortrows, John W. Eaton, 2009/02/12
- Re: issorted & sortrows, Jaroslav Hajek, 2009/02/12
- Re: issorted & sortrows, John W. Eaton, 2009/02/12
- Re: issorted & sortrows, Jaroslav Hajek, 2009/02/12
- Re: issorted & sortrows, dbateman, 2009/02/12
- Re: issorted & sortrows, John W. Eaton, 2009/02/12
- Re: issorted & sortrows, Jaroslav Hajek, 2009/02/12