Consider changing sort() to not update columns in-place #9553

Activity

1. taldcroft

   added

   table

   on Nov 6, 2019
2. pllim

   added

   API changePRs and issues that change an existing API, possibly requiring a deprecation periodPRs and issues that change an existing API, possibly requiring a deprecation period

   Refactoring

   on Nov 7, 2019
3. bsipocz commented on Nov 14, 2019

   

   bsipocz

   on Nov 14, 2019

   Member

   More actions

   @taldcroft - is this something you would have in mind for 4.0?
4. taldcroft commented on Nov 14, 2019

   

   taldcroft

   on Nov 14, 2019

   MemberAuthor

   More actions

   The first time I tried this it broke some tests in ways that were not immediately obvious. I can take another look at this tomorrow and see if the problem is easy to fix. I guess 4.0 would be a good place for an API change like this.
5. taldcroft commented on Nov 16, 2019

   

   taldcroft

   on Nov 16, 2019

   MemberAuthor

   More actions

   I spent some time playing around with this but there is some wacky thing going on with table indexes that are not obvious, so deferring this for now. For the record, this passes current tests:

   diff --git a/astropy/table/table.py b/astropy/table/table.py
   index b157a726b..202a41471 100644
   --- a/astropy/table/table.py
   +++ b/astropy/table/table.py
   @@ -2922,17 +2922,10 @@ class Table:
                # Make a new sorted column.  This requires that take() also copies
                # relevant info attributes for mixin columns.
                new_col = col.take(indexes, axis=0)
   +            if col.info.indices:
   +                new_col = col.info.slice_indices(new_col, indexes, len(col))
    
   -            # First statement in try: will succeed if the column supports an in-place
   -            # update, and matches the legacy behavior of astropy Table.  However,
   -            # some mixin classes may not support this, so in that case just drop
   -            # in the entire new column. See #9553 and #9536 for discussion.
   -            try:
   -                col[:] = new_col
   -            except Exception:
   -                # In-place update failed for some reason, exception class not
   -                # predictable for arbitrary mixin.
   -                self[col.info.name] = new_col
   +            self[col.info.name] = new_col
    
            if sort_index is not None:
                # undo index freeze
   

   However, it fails this test that the column is actually a new object:

   t = simple_table()[[2, 1, 0]]
   t.add_index('a')  # Things work as expected without the index
   ta = t['a']
   print(id(t.columns['a']))
   t.sort('a')
   print(id(t.columns['a']))
   print(ta is t['a'])  # gives True, should be False
   
6. taldcroft commented on Apr 2, 2020

   

   taldcroft

   on Apr 2, 2020

   MemberAuthor

   More actions

   I'm closing. Looking back at this I don't see a really strong driver.
7. taldcroft

   closed this as completedon Apr 2, 2020
8. pllim

   added

   wont-fix

   on Apr 2, 2020