ENH: safety bounds checks for take funcs
State: closed

as per #3028; haven't done vb_suite yet but presumably we're ok paying the price for safety; will run to see what the hit is though


Labels:

Comments:
Hmm, I'm not sure this is going to pass muster with the vbench suite. That's a lot of extra comparisons. Those take functions were never intended to be exposed to arbitrary user input 

@wesm are you aware of user facing possibily for neg indicies anywhere except `Dataframe.take, generic.take` and `_ixs (old icol)` (which `.ix` and friends calls)?

I don't think so. Easiest would be sanitizing take input to Series/DataFrame.take etc. 

done #3027

i'll test it and see...right now it looks like i have a non-optimal bounds checking in the 2d_multi case so I'm fixing that, but in the 2d_axis0 and 2d_axis1 cases the overhead doesn't seem very much; if we decide to skip merging that's ok (i had this lying around for awhile anyway)

@jreback it seems like your fix will solve the handling of properly in-range negative indices, but (unless you add more checks) it won't help if someone provides a wildly out of bounds index? anyway, if you add those checks and we can guarantee that's the only user-facing code that allows indexes to pass into the cythonic takes, then i'm fine not merging this, but I'm really just not sure how sure we are about that

@stephenwlin hmm...that's easy to add, just raise an IndexError.....since user-facing code goes thru here then (unless there are other cases), should be good.....let me revise

ok, well, I'll leave this PR here just in case, but if someone goes through the effort of verifying that we're ok without this, i'm fine with that too

ok.. #3027 is updated to handle neg and out-of-bounds for `take`, just waiting for travis to build...(so wait a while!)

here's the vb_suite impact (+- 10% removed)

```
timeseries_asof_single                      0.0566     0.0513     1.1029
groupby_multi_different_numpy_functions    24.9161    22.1570     1.1245
groupby_multi_different_functions          24.9543    22.0737     1.1305
unstack_sparse_keyspace                     2.1081     1.8639     1.1310
groupby_multi_python                       78.9485    69.8002     1.1311
groupby_multi_series_op                    28.0711    24.8115     1.1314
series_align_int64_index                  211.3378   182.9379     1.1552
stat_ops_level_frame_sum_multiple          19.1902    16.4143     1.1691
stat_ops_level_series_sum_multiple         17.5334    14.8450     1.1811
concat_series_axis1                        90.0609    75.5053     1.1928
series_align_left_monotonic                39.2561    32.7489     1.1987
timeseries_asof_nan                        17.9400    14.8801     1.2056
timeseries_asof                            18.9608    15.6736     1.2097
reshape_pivot_time_series                 357.1150   292.3658     1.2215
reindex_multiindex                          1.9434     1.5629     1.2435
reshape_unstack_simple                      6.0946     4.8971     1.2445
sparse_series_to_frame                    344.4140   273.0908     1.2612
reindex_daterange_pad                       0.5872     0.3029     1.9386
reindex_daterange_backfill                  0.5915     0.3024     1.9558
```

not quite sure yet why reindex_daterange_pad and reindex_daterange_backfill are impacted so much

I think we should leave the bounds checks out-- probably just want to have one set of "fast but unsafe" take functions and "safe" take functions that can accept arbitrary user input (or something).

@wesm ok, I have no objection to that actually

vetoed by wes.