0e69c4db introduced asv regression #3232

Activity

1. mdboom

   added

   Performance

   on Dec 18, 2014
2. taldcroft commented on Dec 19, 2014

   

   taldcroft

   on Dec 19, 2014

   Member

   More actions

   I'll investigate this and the other table regression over the next week.
3. taldcroft commented on Dec 25, 2014

   

   taldcroft

   on Dec 25, 2014

   Member

   More actions

   As expected this is due to the addition of the Python __getitem__ method, which takes an ndarray view of the column if the item is an int. Timing the column item access for a 1-d Column shows 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 a Column instance 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!
4. taldcroft commented on Jan 15, 2015

   

   taldcroft

   on Jan 15, 2015

   Member

   More actions

   @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.
5. mhvk commented on Jan 15, 2015

   

   mhvk

   on Jan 15, 2015

   Contributor

   More actions

   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, in Column.__new__, one can view the ndarray appropriately.

   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.
6. taldcroft commented on Feb 18, 2015

   

   taldcroft

   on Feb 18, 2015

   Member

   More actions

   @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?
7. embray commented on Feb 18, 2015

   

   embray

   on Feb 18, 2015

   Member

   More actions

   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.
8. embray commented on Aug 12, 2015

   

   embray

   on Aug 12, 2015

   Member

   More actions

   This was basically fixed by #3930 . However, #3915 has demonstrated the need for further workaround to this, and in fact implements get_item methods 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 via super(), 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 calling PyArray_Type.tp_as_mapping.mp_subscript for 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.
9. bsipocz commented on May 10, 2017

   

   bsipocz

   on May 10, 2017

   Member

   More actions

   Closing this as #3930 is solved this and #3915 also seems to be addressed. We may want to revisit this and reopen once asv is up and running again and this turns out to be an issue with current master.
10. bsipocz

    closed this as completedon May 10, 2017