Table indexing should not affect Mixin classes except through info - #4222

With #4075 in place, it turns out to be reasonably trivial to remove the index-related cruft in [Quantity|Time].__getitem__. Passes all the table tests, at least locally. I can make this a PR to #4075 if that helps get it in still. cc @embray, @taldcroft.

I rebased now that #4075 is merged, and this PR can be seen much more simply. Ideally, this would still go in 1.10; I'm labeling it as a bug, though it is more of a minor design flaw.

@taldcroft - could you have a quick look at whether this is OK from the Table perspective?

@mhvk - will do, hopefully tomorrow.

@mhvk - this looks fine. The original implementation was intended to allow for propagating the index when a single column is sliced as opposed to the entire table. However that functionality is not tested (apparently) nor in the documentation. I agree that enforcing separation of concerns for mixins is a higher priority, and I can't think of a strong use case for needing the index on an individual sliced column.

Does this have a performance impact on Quantity __getitem__?

Does this have a performance impact on Quantity __getitem__?

The addition introduced in #3915 should have made __getitem__ slower, since it creates the info object by checking whether there are indices. I don't quite know how much of a slow-down this would have been, but certainly removing the check for info.indices should return the speed to what it was before.

Yes, I noticed the info creation.

I'm fine with merging this if you are happy with it.

@astrofrog @embray - There is a question about getting this into 1.1. I would not classify this as a bug but since this change only fixes a performance regression from the user perspective. In theory we can put this into the 1.1.1 release, but 1.1 would be good.

@taldcroft - yes, I think this is good to go as is; only simplifications to the code!
Agreed that it is not strictly a bug, but more a fixup of something that went in already; would be nice if it could still be part of 1.1.

Am merging this, since @taldcroft was OK with it. @astrofrog - if it is really unhandy to get this in 1.1.0 still, then just change the milestone.

backported to v1.1.x in c5ece2d

@mhvk

The addition introduced in #3915 should have made __getitem__ slower, since it creates the info object by checking whether there are indices.

That's what #4075 was about.

@embray - this is actually a little different, since I want to avoid, e.g., a Quantity.info object being created even if the quantity is not part of a table.