Repository navigation
0e69c4db introduced asv regression #3232
Description
Activity
I'll investigate this and the other table regression over the next week.
As expected this is due to the addition of the Python
__getitem__method, which takes an ndarray view of the column if theitemis an int. Timing the column item access for a 1-dColumnshows a factor of 10 slowdown relative to 0.4.2. (Yes, we need a performance test which is basically just accessing a single element of a column).I thought I might be clever and inject a
__getitem__onto aColumninstance only when it gets created and the number of dimensions is > 1.def __col_getitem__(self, item): print('Here in col_getitem') if isinstance(item, INTEGER_TYPES): return self.data[item] # Return as plain ndarray or ma.MaskedArray else: return super(BaseColumn, self).__getitem__(item) def __new__(cls, data=None, name=None, dtype=None, shape=(), length=0, description=None, unit=None, format=None, meta=None, copy=False): ... if self.ndim > 1: self.__getitem__ = types.MethodType(__col_getitem__, self)Annoyingly this does not work. Any clues?
In [1]: from astropy.table import * In [2]: c = Column(np.arange(100).reshape(20,5)) In [3]: c.__getitem__(3) Here in col_getitem Out[3]: array([15, 16, 17, 18, 19]) In [4]: c[3] Out[4]: <Column name=None unit=None format=None description=None> array([15, 16, 17, 18, 19])If the choice is between the issue in #3095 and a factor of 10 performance for integer item access in the typical 1-d case, I might consider backing out #3095 and living with it. But hopefully a better solution can be found!
@astrofrog @mhvk - I think the performance regression introduced by #3095 is pretty serious and we should seriously considering backing it out for 1.0 (unless of course we can figure a way to keep that patch functionality without impacting performance).
As I mentioned above, #3095 fixes a bug that it is more in the category of corner case, while performance for integer access of column values is front-and-center in terms of normal use cases.
I think the way to solve it is to have a separate column class for the case where each item is multidimensional, in which the only difference would be that there is a
__getitem__method. Then, inColumn.__new__, one can view thendarrayappropriately.Anyway, that as a possible solution. Since it is not trivial, I'm OK with pulling #3095 out for now, and reraising it as an issue.
@astrofrog @embray - my fault for not milestoning this properly, but I just remembered about this issue today. I really would like to back out #3095 since it introduces a serious performance regression for a common use case. Probably too late for 1.0, but hopefully there will be a bug fix release soon after where we can take out #3095?
Was just talking to @astrofrog about this, and we're leaning toward maybe waiting 'til 1.0.1 to sort this out, as most users are unlikely to actually notice the performance regression.
This was basically fixed by #3930 . However, #3915 has demonstrated the need for further workaround to this, and in fact implements
get_itemmethods so that it does not have to override (and hence slow down)__getitem__for the common cases.I have a crazy idea that might significantly ease this problem, and also possibly do away with the need for the hack in #3930. What if we define mixin classes that just define the
__getitem__implementation? From my own experimentation it seems that most of the overhead of defining__getitem__in the subclass relates to looking up'__getitem__'in the class__dict__, as well as looking up the base implementation (whether viasuper(),np.ndarray.__getitem__, or a global variable referencing the former). Implementing a type in Cython with a__getitem__slot eliminates much of this overhead. Then, directly callingPyArray_Type.tp_as_mapping.mp_subscriptfor the general case takes care of the rest. I have a prototype demonstrating the basic concept, and my timing shows that it's just as fast as the base implementation for the most common case. I'll try to have something up in a bit.
I'm not saying this an issue to be resolved -- just reporting it FYI.
0e69c4d appears to have introduced a major speed regression in many of the
io.asciibenchmarks:It's a very simple change -- I suspect most of the time is just from adding a
__getitem__vs. not having one. I'm not sure if there's a way to fix the bug in another way, or if it matters. Like I say, just reporting it at this point.