Table with mixin column with SkyCoords can't be sorted

Description

A table that contains a mixin Column with Coordinates cannot be sorted.
I suspect that the underlying reason is the same as in #5745, but that issue is are least
listed as a caveat in the docs: http://docs.astropy.org/en/latest/table/mixin_columns.html#details-and-caveats

Unless there is work on SkyCoord under way, I suggest we add this issue to the caveats.
A related Feature request is: #6348

Expected behavior

Sort the table.

Actual behavior

TypeError: 'SkyCoord' object does not support item assignment

Steps to Reproduce

from astropy.coordinates import SkyCoord
from astropy.table import Table
import astropy.units as u

t = Table({'a':[2,1], 'b':SkyCoord(["1:12:43.2 +1:12:43", "1 12 43.2 +1 12 43"], unit=(u.deg, u.hourangle))})
t.sort('a')

System Details

Linux-3.10.0-1062.1.2.el7.x86_64-x86_64-with-centos-7.7.1908-Core
Python 3.7.3 (default, Mar 27 2019, 22:11:17)
[GCC 7.3.0]
Numpy 1.16.4
SciPy not installed
astropy 4.0.dev26779

Add docs that tables with non-mutable mixins cannot be sorted

@taldcroft

cc @taldcroft @mhvk @eteq @adrn

Hmm, that is actually a bit sad, since as long as one doesn't sort on the SkyCoord, it is very clear what the SkyCoord should do, there being no new frame creation involved. Indeed, it is trivially possible to get the sorted new column by just indexing the SkyCoord with the sorted indices. In principle, this could also be done in-place, perhaps by adjusting the ShapedLikeNDArray class.

Yeah, I was not sure if this is a feature request or a bug... In either case, the minimal response is to put it in the docs. If you think it's easy to fix, then we should do that, of course.

I've been advocating for item assignment in SkyCoord for a long while, even if it is only exposed in some limited "safe/controlled" context. This makes vstack and join fail for SkyCoord as well.

OTOH this seems to work for sort, i.e. punt on the in-place update:

diff --git a/astropy/table/table.py b/astropy/table/table.py
index 6561100da..19408dcab 100644
--- a/astropy/table/table.py
+++ b/astropy/table/table.py
@@ -2919,7 +2919,11 @@ class Table:
             sort_index._frozen = True
 
         for col in self.columns.values():
-            col[:] = col.take(indexes, axis=0)
+            new_col = col.take(indexes, axis=0)
+            try:
+                col[:] = new_col
+            except Exception:
+                self[col.info.name] = new_col
 
         if sort_index is not None:
             # undo index freeze

@taldcroft

@taldcroft - out of curiosity, why is the new column not always used? (since copying back adds extra time.) Is it to keep metadata up to date? The current way also means any earlier links to columns will now refer to sorted data, but I'm not sure that isn't more of an anti-feature...

The current behavior was based on how numpy behaves both with respect to sorting a single array in place and sorting a structured array (with tbl.sort(order='sort_col')). In the latter case in particular a ref to a column is not lost during the sort operation.

My guess was that pandas would do the same, but I just tried df.sort_values('sort_col', inplace=True) and found that it does indeed make new column references.

So there is not an unambiguous precedent, though numpy and astropy Table have historically kept references. BTW, for Table it used to be the case that replacing a column in the table was slow, but no longer.

One thing I that I just appreciated is that take is actually make a deep copy of all the info attributes (e.g. meta). From that perspective we are already paying for a full independent copy of the Column or mixin, so maybe just using the new column always is better?

This would be a real API change. Maybe try to get this into 4.0 as a bug fix for improved mixin compatibility?

I think this would be good for 4.0. It's annoying in the sense that it hits you to late in the process (after your table has been created when you use it).

we're quite late in the process, but if you can open a very quick PR, that agrees with the table maintainers then yes, squeeze it in 4.0.

OK, working on something that we can at least review and make a decision.

Thanks for an incredibly fast turn-around! I found the bug Tuesday afternoon, reported Tuesday evening, took yesterday off, and now I come back to my office to install current astropy master and can just continue my analysis where I left it on Tuesday.

Someone ran into a similar issue during AAS 235 workshop using Astropy 4.0, where this is reportedly fixed. Expect a new issue about this soon.

Navigation Menu

Repository navigation

Table with mixin column with SkyCoords can't be sorted #9536

Description

Expected behavior

Actual behavior

Steps to Reproduce

System Details

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Table with mixin column with SkyCoords can't be sorted #9536

Description

Description

Expected behavior

Actual behavior

Steps to Reproduce

System Details

Activity

pllim commented on Nov 5, 2019

mhvk commented on Nov 5, 2019

hamogu commented on Nov 5, 2019

taldcroft commented on Nov 5, 2019

mhvk commented on Nov 5, 2019

taldcroft commented on Nov 6, 2019

taldcroft commented on Nov 6, 2019

hamogu commented on Nov 6, 2019

bsipocz commented on Nov 6, 2019

taldcroft commented on Nov 6, 2019

hamogu commented on Nov 7, 2019

pllim commented on Jan 5, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions