Fix replace_column for table sub-classes that have non-conventional initializers #8702
Conversation
astrofrog
added
table
Bug
Affects-dev
|
@astrofrog - some quick comments:
This gets back to a comment I made on the original TimeSeries PR that I think it should be able to be created without time inputs. IIRC the Table code assumes in some places that a subclass can be created with a standard data input. (This is one such place). I think @mhvk had made reference to some comp-sci principle that a subclass should pass all the tests of the parent class. I'm not sure I'd go all the way on that, but there is something to be said for a |
|
Liskov substitution principle - I'm also not sure whether it is always practical to hold, but found it very helpful in thinking about classes. |
| t = self.__class__([col], names=[name]) | ||
| # We deliberately use ``Table`` here since it should be sufficient | ||
| # and it is just meant as a temporary container for the column | ||
| t = Table([col], names=[name]) |
There was a problem hiding this comment.
I guess the goal here is to ensure the input can be turned into a column. A fairly safe replacement would seem to be
if not isinstance(getattr(col, 'info', None), BaseColumnInfo):
col = Column(col, name=name)
cols = OrderedDict(self.columns)
cols[name] = col
self._init_from_cols(cols.values())
There was a problem hiding this comment.
Here's a test case (using current master) that should be included. I don't think the above would pass this:
In [11]: t = simple_table()
In [12]: b = np.ma.MaskedArray([4, 5, 6])
In [13]: b[1] = np.ma.masked
In [14]: t.replace_column('b', b)
INFO: Upgrading Table to masked Table. Use Table.filled() to convert to unmasked table. [astropy.table.table]
In [15]: t
Out[15]:
<Table masked=True length=3>
a b c
int64 int64 str1
----- ----- ----
1 4 c
2 -- d
3 6 e
I've been thinking about a better solution but nothing great has come to mind. I believe that the implementation currently in this PR will work, I'm just a little unhappy about the case where a Quantity gets converted to Column and then back to Quantity because of Table. I thought about using QTable there instead, but that is worse because it can end up sending Quantity objects into Table._init_from_cols, which is not expecting that and will leave them as Quantity.
So maybe a TimeSeries method overload is best, where you use QTable. Or better yet fix TimeSeries so it works with this existing method. 😄
There was a problem hiding this comment.
Maybe a combination of the two - use the Table only for objects that don't have BaseColumnInfo? I.e., something like
if not isinstance(getattr(col, 'info', None), BaseColumnInfo):
col = Table([col], names=[name])[name]
...
|
Btw, there is also the related issue from #8077 with |
pllim
removed
Affects-dev
|
I don't think this only "affects dev" anymore, so I removed the label. This now needs a change log under bug fix. |
|
BTW there will be an easier solution with #8789. |
|
@astrofrog - Now that #8789 is being merged, could you please revise this PR? |
6 participants
This is an attempt at a fix for #8642, but I need @taldcroft's input.
Basically
replace_columndoesn't work properly forTimeSeriesbecause theTimeSeriesinitializer requires some kind of times to be passed to it. But in any case, it seems like we don't really need to useself.__class__inreplace_columnunless I'm missing something?I don't fully understand why this is needed in the first place and why we can't simply modify the table column dictionary in-place, so @taldcroft if you could provide some background, that would be really useful, and I can try and add some comments in the code to explain.
If this is not an acceptable solution, I'll just overload
replace_columninTimeSeriesinstead.