Repository navigation
When slicing a table mixin column, do not copy indices by default. - #10890
Conversation
But rather only do it if the table is sliced.
|
(Failure somewhat obscure -- I'll look into it) |
9d4ad16 to
92b8705
Compare
|
@taldcroft - the small bug uncovered by the doctest (ouch!) was because in converting a Column to a Quantity for QTable, the path was not quite the same and the indices were not initialized quite right any more. I now got all tests to pass and included new ones (including one that covers the case identified by the doctest). For now, I milestoned for 4.2, partially out of worry this breaks something unexpected. But I could go with 4.0.4 too since this does seem a bug fix. Note: I briefly tried to set the default for |
|
@mhvk - I think this is fine, but I don't think that it is a replacement for #10889, which is explicitly about behavior for getting a scalar item For instance, the info attribute can have |
| astropy.table | ||
| ^^^^^^^^^^^^^ | ||
|
|
||
| - Fix a bug where slices of columns still carried indices even if they lost |
There was a problem hiding this comment.
I think this should be called an API change. It was not wrong or unexpected behavior before given the original design in BaseColumnInfo (that indices were copied), but we are deciding now that it is more logical / better performance to change the design. Also, although it would be surprising, someone might be expecting the old behavior.
There was a problem hiding this comment.
OK to mention it as an API change. The reason I thought it might be more like a bug is that parent_table is not copied, which seems to make the indices rather useless. Furthermore, if the slice is taken on the table, the indices are not just copied, but carry information about the slice, so the behaviour is not quite consistent (see below). If it is a bug, in principle, we could backport this to 4.0.
In [63]: ts_a
Out[63]:
<TimeSeries length=5>
time flux
mJy
object float64
----------------------- -------
2016-03-22T12:30:31.000 1.0
2016-03-22T12:30:34.000 4.0
2016-03-22T12:30:37.000 5.0
2016-03-22T12:30:40.000 3.0
2016-03-22T12:30:43.000 2.0
In [64]: ts_a['time'].info.indices
Out[64]:
[ time rows
----------------------- ----
2016-03-22T12:30:31.000 0
2016-03-22T12:30:34.000 1
2016-03-22T12:30:37.000 2
2016-03-22T12:30:40.000 3
2016-03-22T12:30:43.000 4]
In [65]: ts_a[:3]['time'].info.indices
Out[65]:
[Index slice (0, 3, 1) of
time rows
----------------------- ----
2016-03-22T12:30:31.000 0
2016-03-22T12:30:34.000 1
2016-03-22T12:30:37.000 2
2016-03-22T12:30:40.000 3
2016-03-22T12:30:43.000 4]
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
Basically good with the change to the change log. I think that text should just focus on the change which is that if you slice a column (Column or mixin column) that has an index (info.indices), the index is no longer copied into the sliced output.
I would disagree with this characterization. The actual change is in |
|
Well, it is in But what version should we aim at? I.e., should we consider dropping |
|
I guess I tend to be pretty conservative about what to call a bug if it is going in a bugfix release. Basically the question is whether it is credible that some user out there is taking advantage of this existing behavior and their code will break after installing a bugfix release. I take that contract very seriously. It seems very unlikely that anyone is using that, but it is tough to prove otherwise. Usually to me for an API change like this to get put into a bug fix, I would say it is OK if the original behavior was so wrong that nobody could be relying on that behavior. Here that is not unambiguously the case, but I agree it is in the grey zone. |
92b8705 to
73c0a6f
Compare
|
@taldcroft - I created an entry in API changes instead of bug fixes. While writing it, I realized again that with this change, mixin columns now behave more similarly to regular columns - indeed, the test I added checks that explicitly. That perhaps suggests a bit more the previous behaviour was not intended. That might argue for a bug-fix in LTS after all, though on the other hand it is not something that will bother many people, so also fine to just go with just 4.2 (as I left it now). |
But rather only do it if the table is sliced.
Alternative to #10889
for now, just to discuss and check that all tests still pass (have to run...)EDIT: the advantage here is that it solves the problem at the table level.
EDIT 2: this actually affects regular columns as well...