Skip to content

Conversation

jorisvandenbossche
Copy link
Member

This line was added in #43977, while the docstring and type information (as added in #43977) indicates this is already the case.

@jbrockmendel
Copy link
Member

we have a lot of should-be-unnecessary ensure_platform_int calls. these should be extremely cheap, so haven't been bothering to remove.

@jorisvandenbossche
Copy link
Member Author

so haven't been bothering to remove.

And in the past I bothered making a fast-path take_1d which explicitly removed it. It did show up in reindex benchmarks IIRC (#40246, #39692)

@jorisvandenbossche
Copy link
Member Author

Digging up the benchmarks. Doing one specific for take_1d:

from pandas.core.array_algos.take import take_1d
arr = np.arange(10000)
idx = np.arange(100, dtype=np.intp)

In [5]: %timeit take_1d(arr, idx, allow_fill=False)
498 ns ± 0.77 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  # <-- master
465 ns ± 0.962 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  # <-- PR

This is of course about nanoseconds, so in absolute terms only a tiny difference. But because this function is used repeatedly in things like unstack and reindex, it does make a noticeable difference. Eg on the unstack case as used in #40246, it gives ~ 3-5% difference (which is still small, but IMO worth it since this is actually removing a single line, and the constraint is explicitly documented / in the type information)

@jbrockmendel
Copy link
Member

sure no objection

@jreback jreback added 32bit 32-bit systems Performance Memory or execution speed performance labels Nov 21, 2021
@jreback jreback added this to the 1.4 milestone Nov 21, 2021
@jreback
Copy link
Contributor

jreback commented Nov 21, 2021

no objection, can you rebase

@jorisvandenbossche jorisvandenbossche merged commit 3a25cb9 into pandas-dev:master Nov 22, 2021
@jorisvandenbossche jorisvandenbossche deleted the am-take1d-novalidateindices branch November 22, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
32bit 32-bit systems Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants