[CLN] Make more of numpy_helper unnecessary
State: closed


Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Clean

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22344?src=pr&el=h1) Report
> Merging [#22344](https://codecov.io/gh/pandas-dev/pandas/pull/22344?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/cf70d11a25ec59a5b630921d86759c7fa49884db?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/22344/graphs/tree.svg?height=150&width=650&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/22344?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #22344   +/-   ##
=======================================
  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/22344?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/22344?src=pr&el=footer). Last update [cf70d11...29972ea](https://codecov.io/gh/pandas-dev/pandas/pull/22344?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Do you know if there is any benefit in using `get_value_1d`? (what was the original reason for using it?)
> Do you know if there is any benefit in using get_value_1d? (what was the original reason for using it?)

This and #22329 were both spun off of a branch that was seeing if that could be made unnecessary.   
For the most part, we can replace `get_value_1d(arr, i)` with `arr[i]` just fine.  The one place in the tests where that doesn't work is a code both that goes through `SparseArray.__getitem__`, because that leads to a `RecursionError`.  IIUC several other pandas objects used to subclass `np.ndarray`, so may have needed the same workaround to effectively call `ndarray.__getitem__(arr, i)`.

After this and #22329 I'll be ready to make a PR to get rid of numpy_helper altogether, since `get_value_1d` and `set_value_1d` can be made unnecessary.
OK. It seems to have been introduced to make sure Series getitem would return numpy scalars: https://github.com/pandas-dev/pandas/commit/96375da37cde2072caaf2e46f16174725aa0e492. But I suppose that is now covered by the tests.
> pls perf check this

`lib.infer_dtype` with mixed-dtype should go through one of the affected paths:

```
In [3]: arr = np.array(range(10000), dtype='O')

In [4]: arr[100] = 'foo'
```

master:
```
In [10]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 21.5 µs per loop

In [11]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 25.6 µs per loop

In [12]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 19.9 µs per loop
```

PR:
```
In [7]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 20.1 µs per loop

In [8]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 24.3 µs per loop

In [9]: %timeit pd.lib.infer_dtype(arr)
10000 loops, best of 3: 19.9 µs per loop
```

Timings look good.
thanks. can you do a PR with those as asv's? (if not already there)
>  can you do a PR with those as asv's?

Sure.  I think for low-level cython stuff we may need an extra layer (in cython) to call these tiny functions in a loop.
> Sure. I think for low-level cython stuff we may need an extra layer (in cython) to call these tiny functions in a loop.

That should not be needed for `infer_dtype` (and also for the others I would try to check what the actual "higher level" usage is)
> That should not be needed for infer_dtype

You're right about infer_dtype, but I'm suggesting we directly benchmark the affected code.
IMO we should always try to test higher level code / (semi) public APIs (to the extent possible of course). So if possible (if there is a reasonable higher level code, such as `infer_dtypes` already is) I would not directly benchmark the affected code. Otherwise that makes the benchmarks very internals-specific (making it more difficult to change internals, or to compare to alternative implementations)
> So if possible (if there is a reasonable higher level code, such as infer_dtypes already is) I would not directly benchmark the affected code.

My issue with this is that the code affected by this PR constitutes a tiny fraction of the runtime of `infer_dtypes`.  asv results are mostly noise anyway; more focused benchmarks can chip away at that problem.

 > Otherwise that makes the benchmarks very internals-specific (making it more difficult to change internals, or to compare to alternative implementations)

Part of the motivation I have in mind comes from a change I was looking at to implement pure-cython implementations of `np.zeros(n, dtype=np.float64)`.  The benchmark I wrote for that (unfortunately now-deleted) branch looked something like:

```
def time_zeros(size=10000, loops=1000, version=0):
     if version == 0:
          for n in range(loops):
              out = np.zeros(size, dtype=np.float64)
    else:
        for n in range(loops):
            out = zeros1d(size)
    return out
```

With comments documenting the runtime measurements for each version.  The idea being to measure+document the alternative implementations.