Re: problem with sorting

[David Bateman]

John W. Eaton wrote:
> I noticed this problem:
> 
>   octave:1> typeinfo (int8 ([1,2;3,4]))
>   ans = int8 matrix
>   octave:2> typeinfo (sort (int8 ([1,2;3,4])))
>   ans = matrix
> 
> I think this should return an int8 matrix, to match the type of the
> argument passed to sort.
> 
> In src/DLD-FUNCTIONS/sort.cc, we dispatch on type and only handle
> NDArray, ComplexNDArray, charNDArray, and Cell objects.  Integer
> values are converted to NDArary objects, then sorted.
> 
> How should we fix this?  Should we add a sort method to the
> octave_value class hierarchy?  That way the dispatching can be handled
> by virtual methods in the octave_value class.  Does anyone see
> disadvantages to doing that?
> 
> jwe
> 

No it makes lots of sense to me, In that case the code

enum sortmode { UNDEFINED, ASCENDING, DESCENDING };

template <class T>
class
vec_index
{
public:
  T vec;
  octave_idx_type indx;
};

template <class T>
bool
ascending_compare (T a, T b)
{
  return (a < b);
}

template <class T>
bool
descending_compare (T a, T b)
{
  return (a > b);
}

template <class T>
bool
ascending_compare (vec_index<T> *a, vec_index<T> *b)
{
  return (a->vec < b->vec);
}

template <class T>
bool
descending_compare (vec_index<T> *a, vec_index<T> *b)
{
  return (a->vec > b->vec);
}


>From the top of sort.cc as well as the template functions mx_sort and
mx_sort_indexed, should probably be taken into src/ in some manner, so
that all types can make use of it.

Also, I think the call needs to respect the possible parameters that
sort returns. That is I'd make the method look something like

octave_value_list sort (const octave_value &v, const int nargout = 1,
const int dim = -1, const int mode = 1);

Where a dim = -1 specifies to sort along the first non-sngleton. With
that most of the sort code can be split out of Fsort. Also note that for
int8 sort the existing sort class in oct-sort.h make it pretty easy to
add the above function. It would look something like

#if !defined (CXX_NEW_FRIEND_TEMPLATE_DECL)
bool
ascending_compare (octave_int8 a, octave_int8 b);

bool
ascending_compare (vec_index<octave_int8> *a, vec_index<octave_int8> *b);

bool
descending_compare (octave_int8 a, octave_int8 b);

bool
descending_compare (vec_index<octave_int8> *a, vec_index<octave_int8> *b);

octave_value_list
mx_sort (ArrayN<octave_int8> &m, int dim, sortmode mode);

octave_value_list
mx_sort_indexed (ArrayN<octave_int8> &m, int dim, sortmode mode);
#endif

template class vec_index<octave_int8>;
template class octave_sort<vec_index<octave_int8> *>;

octave_value_list
octave_int8::sort (const octave_value &v, const int nargout, const int
dim, const int mode)
  {
    if (nargout < 2)
      return mx_sort (v.int8_value(), dim, mode);
    else
      return mx_sort_indexed (v.int8_value(), dim, mode);
}

Most of the code to allow this plan is already in place.. Note that for
performance reasons the double versions of mx_sort and mx_sort_indexed
are specialized template function though.... BTW, do we need to still
consider the case where CXX_NEW_FRIEND_TEMPLATE_DECL is not defined? If
we don't then basically half the code above disappears..



D.