CLN: de-duplicate index validation code
State: closed

There are currently 3 nearly-identical versions of this code.  I'm pretty sure the strictest one is the most-correct, so that is made into the only one.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: Clean

Comments:
yeah these are in criticial paths of perf. pls do a check.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=h1) Report
> Merging [#22329](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b5d81cfe43eeccfc3641aa9578097f726da9ce9d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/22329/graphs/tree.svg?token=eZ4WkYLtcO&width=650&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22329   +/-   ##
=======================================
  Coverage   92.05%   92.05%           
=======================================
  Files         169      169           
  Lines       50709    50709           
=======================================
  Hits        46679    46679           
  Misses       4030     4030
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.46% <ø> (ø)` | :arrow_up: |
| #single | `42.25% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=footer). Last update [b5d81cf...49ac36b](https://codecov.io/gh/pandas-dev/pandas/pull/22329?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> yeah these are in criticial paths of perf. pls do a check.

Running asv now.

> I don't think you need the question mark in except? ? (since -1 can never be actually returned from the function without it being an error)

The `?` is there purely out of habit.  Is there a cost to having it there?

> what is exactly the difference between the current versions?

Corner case handling.  e.g. if `len(arr) == 10` and `i = -4` they will all increment `i += len(arr)` and be OK.  But if `i = -14` only the strict one will check that `i + len(arr)` is still negative.
asv results:

```
asv continuous -f 1.1 -E virtualenv master HEAD -b frame
[...]
       before           after         ratio
     [ffae1587]       [7d2e4330]
+      9.97±0.5ms       16.1±0.3ms     1.61  frame_methods.Apply.time_apply_lambda_mean

asv continuous -f 1.1 -E virtualenv master HEAD -b frame
[...]
       before           after         ratio
     [ffae1587]       [7d2e4330]
+        894±80μs      2.19±0.04ms     2.46  frame_ctor.FromRecords.time_frame_from_records_generator(1000)

taskset 8 time asv continuous -f 1.1 -E virtualenv master HEAD -b frame
[...]
       before           after         ratio
     [ffae1587]       [7d2e4330]
-        172±10ms        946±100μs     0.01  frame_ctor.FromRecords.time_frame_from_records_generator(None)

time taskset 4 asv continuous -f 1.1 -E virtualenv master HEAD -b frame
[...]
       before           after         ratio
     [ffae1587]       [7d2e4330]
-     2.20±0.02ms      1.01±0.01ms     0.46  frame_ctor.FromRecords.time_frame_from_records_generator(1000)

```
The asv is clearly not really relevant. Can you do a quick check of the impacted functions with a direct call? Eg `get_value_box` is only used in `Index.get_value` and is eg hit when doing

```
In [4]: idx = pd.Index(['a', 'b', 'c', 'd'])

In [5]: s = pd.Series(range(len(idx)), index=idx)

In [6]: idx.get_value(s, 1)
Out[6]: 1
```

You can do a quick `%timeit` before/after (but with bigger index) to see if there is any significant change.

> The ? is there purely out of habit. Is there a cost to having it there?

There is an extra check whether there is an error raised or not. I don't assume this is costly, but since we know that it will always be an error, I think it is more cleanly code-wise to reflect this in the `except`

>  But if i = -14 only the strict one will check that i + len(arr) is still negative.

OK, that sounds like the good change!
I’ll remove the question mark when possible.  At the moment the power
company has decided to do some surprise maintenance, so that might be a
while...

As to the asv, yah it’s not clearly relevant, but it’s low-level code that
gets used a lot so I guessed that running all frame benchmarks would hit
it.  I’ll try your suggestion when the power comes back.

On Tue, Aug 14, 2018 at 12:07 PM Joris Van den Bossche <
notifications@github.com> wrote:

> But if i = -14 only the strict one will check that i + len(arr) is still
> negative.
>
> OK, that sounds like the good change!
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/22329#issuecomment-412982424>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AHtGeGJKyGmQX9uObIMNzyA-3pr8ob_5ks5uQyAEgaJpZM4V7qQO>
> .
>

Indistinguishable:

Master:
```
In [2]: idx = pd.Int64Index(range(10**6))

In [3]: ser = pd.Series(idx, index=idx)

In [4]: %timeit idx.get_value(ser, 1000)
10000 loops, best of 3: 26.6 µs per loop

In [5]: %timeit idx.get_value(ser, 1000)
10000 loops, best of 3: 26.7 µs per loop

```

PR:
```
In [2]: idx = pd.Int64Index(range(10**6))

In [3]: ser = pd.Series(idx, index=idx)

In [4]: %timeit idx.get_value(ser, 1000)
10000 loops, best of 3: 26.8 µs per loop

In [5]: %timeit idx.get_value(ser, 1000)
10000 loops, best of 3: 25.6 µs per loop
```
I don't think your example with `Int64Index` hits the function you modified (note my example used a string index, so it does integer position fallback indexing), but you can check that by adding a pdb trace or print statement to be sure.
```
In [2]: idx = pd.Index([str(x) for x in range(10**6)])

In [3]: ser = pd.Series(range(len(idx)), index=idx)

In [4]: %timeit idx.get_value(ser, 1000)
```

Master:
```
The slowest run took 7246.54 times longer than the fastest. This could mean that an intermediate result is being cached.
1 loop, best of 3: 37.9 µs per loop

The slowest run took 5.44 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 19.5 µs per loop

The slowest run took 5.11 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 18.8 µs per loop

The slowest run took 5.25 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 18.4 µs per loop

The slowest run took 4.27 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 23.6 µs per loop
```

PR:
```
The slowest run took 9550.30 times longer than the fastest. This could mean that an intermediate result is being cached.
1 loop, best of 3: 29.1 µs per loop

The slowest run took 5.77 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 19.1 µs per loop

The slowest run took 5.49 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 19.3 µs per loop

10000 loops, best of 3: 25.7 µs per loop

The slowest run took 4.35 times longer than the fastest. This could mean that an intermediate result is being cached.
10000 loops, best of 3: 18.2 µs per loop
```

@jreback I am not sure that is needed. It should already be covered by other indexing benchmarks that use `get_value` under the hood 
(I mainly here wanted to check the very specific code path to be sure we are checking the correct thing, as a sanity check)
Following this and #22344 it tentatively looks like we'll be ready to get rid of numpy_helper (and chunks of util) altogether.
@jreback gentle ping.  After this we can get rid of a bunch of old numpy_helper code.
thanks!

yeah lots of PRs!