table: unable to replace a column by one with a different type #3809

Activity

1. taldcroft commented on Jun 2, 2015

   

   taldcroft

   on Jun 2, 2015

   Member

   More actions

   @barentsen - I see your perspective, but this type immutability is by design and you find the same behavior in numpy structured arrays. As seen in the traceback you have:

               if isinstance(item, six.string_types):
                   # Set an existing column
                   self.columns[item][:] = value
   

   Since Column is a numpy array subclass this basically defers to whatever numpy wants to do for assigning, which might be broadcasting or whatever, but the array type never changes.

   I think the behavior you propose would have it's own surprises for people. For instance the following example:

   >>> c1 = Column([1.0, 2.0], dtype='f4', name='c1', description='col 1')
   >>> c2 = Column([1.5, 2.5], dtype='f8', name='c2', description='column 2')
   >>> t = Table([c1, c2])
   >>> t['c1'] = t['c1'] + np.sqrt([1, 2])
   >>> t['c1'].dtype.name  # Changed from float32 to float64 which would surprise some people
   float64
   >>> t['c1'] = t['c2'] + t['c1']
   >>> t['c1'].description  # When adding columns metadata taken from leftmost value
   'column 2'   # shouldn't this be 'col 1' ?!?
   

   A 2-line workaround is:

   tbl.columns['col'] = tbl['col'].astype(np.float64)
   tbl = Table(tbl.columns, copy=False)
   

   In theory we could make a method for this like replace_column, or else just document this (not totally obvious) trick. This takes advantage of the pre-existing dictionary of columns that you can update in place. Don't be tempted to stop after the first line, even though your tbl will appear to have been fixed. This is an illusion and downstream breakage will happen!
2. barentsen commented on Jun 2, 2015

   

   barentsen

   on Jun 2, 2015

   ContributorAuthor

   More actions

   Many thanks for the clarification @taldcroft !

   I did end up discovering the tbl.columns['col'] = new_column workaround after some trial and error. A replace_column method would have been far easier to discover however, and would offer a place to document these pitfalls, so I'd be 👍 for adding replace_column.

   Would you be able to clarify why the 2nd line in your workaround is needed? i.e. tbl = Table(tbl.columns, copy=False)
   I have simply been using

   tbl.columns['col'] = tbl['col'].astype('float32')
   tbl.write('table.fits')
   

   ... and table.fits had correctly absorbed the change, i.e. 'col' was a single rather than a double in the FITS table. Was I lucky? When would trouble occur?

   Thanks!
3. barentsen commented on Jun 2, 2015

   

   barentsen

   on Jun 2, 2015

   ContributorAuthor

   More actions

   Oh, by the way:
   tbl = Table(tbl.columns, copy=False)
   raises:
   ValueError: Cannot use copy=False with a dict data input

   so I'm using
   tbl = Table(tbl, copy=False)
4. embray

   added

   table

   on Jun 2, 2015
5. embray commented on Jun 2, 2015

   

   embray

   on Jun 2, 2015

   Member

   More actions

   Does this need to be clarified in the docs? I was a little surprised too, but also see the reasoning (well, I'd be less surprised when Table had an underlying structured array, but now that the data is stored column-wise I'm more surprised).
6. taldcroft commented on Jun 3, 2015

   

   taldcroft

   on Jun 3, 2015

   Member

   More actions

   Without the Table statement you can get into a pickle like this:

   In [3]: t = Table([[1]], names=('a'))
   
   In [4]: t.columns['b'] = t['a']
   
   In [5]: t.columns['c'] = t['a']
   
   In [6]: t
   Out[6]: 
   <Table masked=False length=1>
     a     a     a  
   int64 int64 int64
   ----- ----- -----
       1     1     1
   

   Also, columns have a parent_table attribute that doesn't get set properly.

   The copy=False was working for me, but that must have been on master. Can you try that? Otherwise you just need to use the default copy=True.

   Some of these problems I just mentioned could be fixed, so I might have a look at that to support this better.
7. mhvk commented on Jun 4, 2015

   

   mhvk

   on Jun 4, 2015

   Contributor

   More actions

   Funny to see how one of my first PRs, creating a new column automatically when assignment was to a column that did not yet exist, now comes back to bite! I think the easiest trick to explain is simply

   tbl.remove_column(col)
   tbl[col] = ...
   

   But given that we have add_column and remove_column, the idea of having a replace_column seems sensible. @barentsen - do you want to have a go? It seems it should be relatively straightforward.
8. barentsen commented on Jun 7, 2015

   

   barentsen

   on Jun 7, 2015

   ContributorAuthor

   More actions

   I like the idea of adding replace_column because its docstring would provide a convenient place to document the difference between a "replacement operation" and a "column assignment". I would probably not have opened this issue if replace_column existed, because I did look for a function with such a name at first.

   Unfortunately I'm in the middle of a change of jobs/continents, which prevents me from turning this into a PR in the next few weeks.
9. taldcroft commented on Aug 24, 2015

   

   taldcroft

   on Aug 24, 2015

   Member

   More actions

   See #4090.
10. mhvk

    closed this as completedin #4090on Oct 1, 2015