Improve consistency of Table sorting

What is the problem this feature will solve?

Currentlly there are at least 3 different implementations of sorting columns and none of them seem quite right.

Table.argsort() makes a numpy structured array using .as_array() and calls argsort. So for a Time column this will make an object column with each row as a new Time object. Bad. And Table.argsort exposes a kind kwarg which will cause an exception when sorting the table on a Time column.

astropy/astropy/table/table.py

Lines 3444 to 3445 in aa2c05d

data = self.as_array(names=keys)

else:
table/groups.py uses represent_mixins_as_columns to flatten the sorting table. This is one way to do it but for Time this gives columns with jd1 and jd2 instead of using the info.get_sortable_arrays() machinery.

astropy/astropy/table/groups.py

Line 79 in aa2c05d

if not table_index and isinstance(table_keys, Table):
table/index.py does something different as well, making a list of columns to sort lexically, including a special-case for Time that repeates the get_sortable_arrays code.

astropy/astropy/table/index.py

Line 103 in aa2c05d

new_columns = []

It seems like the ideal way to argsort a list of table columns is to call info.get_sortable_arrays on each one and assemble all the results into a list of arrays which are then passed to np.lexsort. But one problem is that this (once again) would render moot the kind argument of Table.argsort (and Table.sort).

Describe the desired outcome

More unified and consistent approach to sorting table columns.

Additional context

Performance and API changes are potential concerns.

@mhvk

One question for @mhvk - the current implementation of Time.info.get_sortable_arrays() is rigorously correct but relatively slow. So two questions:

Does Time strictly control the internal jd1 and jd2 values to conform to the day_frac() scheme? I think so, but I don't know if there could be corner cases otherwise.
If so, could get_sortable_arrays() simply return jd1 and jd2?

@taldcroft

@taldcroft - I think the internal control of jd1 and jd2 is pretty good these days, but I'm not sure I would completely count on it - especially with rounding to even. Regardless, probably good to keep that as a separate issue - what is there currently is the same as used for time.argsort() so it won't be any slower (and of course is far faster than making an object array!).

@mhvk

@mhvk - Ahah, never mind on the Time argsort. It turns out to be essentially as fast as it can be. I was being misled by the implementation in get_sortable_arrays which gets the remainder by making an entirely new Time object and doing a Time difference (and I'm assuming this is pretty slow):

    def get_sortable_arrays(self):
        """
        Return a list of arrays which can be lexically sorted to represent
        the order of the parent column.

        Returns
        -------
        arrays : list of ndarray
        """
        parent = self._parent
        jd_approx = parent.jd
        jd_remainder = (parent - parent.__class__(jd_approx, format="jd")).jd
        return [jd_approx, jd_remainder]

The implementation in Time.argsort instead just does pure numpy floating point operations:

        jd1, jd2 = self.jd1, self.jd2
        approx = jd1 + jd2
        remainder = (jd1 - approx) + jd2

Should we use that floating point version in get_sortable_arrays?

Yes, we should! See https://github.com/astropy/astropy/pull/11907/files#r662320174 where we discussed why there was a call to self.__class__ also for .argsort - we just forgot to update get_sortable_arrays (maybe we should use get_sortable_arrays in .argsort??)

But one problem is that this (once again) would render moot the kind argument of Table.argsort (and Table.sort).

Looking at the numpy.lexsort docs it states that Structured arrays are sorted lexically by argsort. I cannot find any more specific statement in the argsort docs, but it seems likely that the kind argument does not fully apply if passing in a structured array (as is currently done for Table.argsort). More likely it is just using the default stable sort under the hood (and regardless of our current API docs it has always been doing this).

So I'd be inclined to make our API reflect this reality and just issue a warning if kind is specified as anything other than None, stable, or mergesort. All of these are basically what's going on under the hood.

👍 on simply noting what our API does. I think the larger point of this issue still stands, though: as you suggest on top, one should use .info.get_sortable_arrays and then do a lexsort on those.

Navigation Menu

Repository navigation

Improve consistency of Table sorting #14942

What is the problem this feature will solve?

Describe the desired outcome

Additional context

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	data = self.as_array(names=keys)
	else:

Improve consistency of Table sorting #14942

Description

What is the problem this feature will solve?

Describe the desired outcome

Additional context

Activity

taldcroft commented on Jun 14, 2023

mhvk commented on Jun 14, 2023

taldcroft commented on Jun 14, 2023

mhvk commented on Jun 14, 2023

taldcroft commented on Oct 15, 2023

mhvk commented on Oct 15, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions