[CLN] Make more of numpy_helper unnecessary

[jbrockmendel]

No description provided.

[codecov]

Codecov Report

Merging #22344 into master will not change coverage.
The diff coverage is n/a.

@@           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% <ø> (ø)	⬆️
#single	42.25% <ø> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf70d11...29972ea. Read the comment docs.

[jorisvandenbossche]

Do you know if there is any benefit in using get_value_1d? (what was the original reason for using it?)

[jreback]

pls perf check this

[jbrockmendel]

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.

[jorisvandenbossche]

OK. It seems to have been introduced to make sure Series getitem would return numpy scalars: 96375da. But I suppose that is now covered by the tests.

[jbrockmendel]

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

[jorisvandenbossche]

Timings look good.

[jreback]

thanks. can you do a PR with those as asv's? (if not already there)

[jbrockmendel]

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.

[jorisvandenbossche]

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)

[jbrockmendel]

That should not be needed for infer_dtype

You're right about infer_dtype, but I'm suggesting we directly benchmark the affected code.

[jorisvandenbossche]

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)

[jbrockmendel]

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.