PERF: Always using panda's hashtable approach, dropping np.in1d #36611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
Nice find. Not sure if we should just lower the limit in the condition to keep performance for a low number of elements to check, but I think this is OK |
jreback
requested changes
Sep 24, 2020
There was a problem hiding this comment.
i am pretty sure we have some other isin benchmarks, can you run them.
This is very odd result, I am pretty sure this numpy approach at least when we added this was WAY faster.
could be wrong, but see if you can find the issues that changed this and make sure that this is not in error.
|
I have run all benchmarks which used The only tests with difference were: which I would say is an improvement in overall. However, I have also changed my introduced benchmarks because they had a bias in the first version: most of the elements from the series were not in the
Now the benchmark have another bias: every element from the series is present in Interesting difference to the original benchmark:
My assumption is that the hash-functions for float64 ( ) isn't that great and 1.0, 2.0, 3.0 and so on are too often mapped onto the same hash-value. But this needs further inverstigation. |
See also #28303 |
|
Just to illustrate how bad/severe the issue is. Here is the currently used hash-function for For 1000 keys, khash set/map will use 2048-buckets, thus we need to see the hashes modulo 2048: gives us only 2 different hashes - 0 and 1024, but for int64 there are 1000 different hashes. Which mean that the look-up is O(n^2) for float64. |
|
The most obvious solution would be to use This changed hash-function on top of this branch gave (only floats): The important results are:
It looks promising, however
there are no hash-collisions and that is all Python's dict needs. However, khash has another strategy and such a regular hash function probably means problems (by the way it also means problems for Another alternative which should be considered/tested is murmur-hash (https://github.com/aappleby/smhasher) which is used in gcc's libstdc++ (https://github.com/gcc-mirror/gcc/blob/41d6b10e96a1de98e90a7c0378437c3255814b16/libstdc%2B%2B-v3/libsupc%2B%2B/hash_bytes.cc#L25) as well as in libc++ (https://github.com/llvm/llvm-project/blob/1cfde143e82aeb47cffba436ba7b5302d8e14193/libcxx/include/utility#L977). |
|
not averse to having a better float hash impl but would need some serious benchmarking |
5 tasks
It is hard to tell what brought the speed-up. My first suspect would be #13436, but I'm not sure. However, the performance testing for this PR should probably wait until a judgement is passed for #36729. |
|
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
| param_names = ["dtype", "M"] | ||
|
|
||
| def setup(self, dtype, M): | ||
| self.s = Series(np.random.randint(0, M, 10 ** 7)).astype(dtype) |
There was a problem hiding this comment.
IIUC we can expect big perf differences if the values here are clustered, e.g. if they are all negative or all >M in the extreme cases. Do we need to measure any non-uniform cases?
There was a problem hiding this comment.
@jbrockmendel You are right, the test is a little bit naive. We should test at least (I have pushed them):
- average (random) case with elements in the look-up-table
- average (random) case with elements not in the look-up-table
- monotone series with elements in the look-up-table (better cache utilization for numpy's approach)
- monotone series with elements not in the look-up-table (better cache utilization for numpy's approach)
it looks as if float32/float64 are somewhat problematic, hopefully after #36729 it will become clearer what is going on.
realead
force-pushed
the
speedup_isin
branch
from
b93b1e4
to
1a85bb8
Compare
|
Current time comparisons: One should see, how it looks once #36729 is resolved - right now float64/float32 seems to be problematic. |
|
this looks way better even w/o #36729, but if you can re-run and will see. |
jreback
requested changes
Nov 14, 2020
pandas/core/algorithms.py
Outdated
| else: | ||
| f = np.in1d | ||
| elif is_integer_dtype(comps): | ||
| if is_integer_dtype(comps.dtype): |
There was a problem hiding this comment.
can you but some commentary / link here to why we are not using np.int1d
…the search in the sorted arrays will have too few cache misses
realead
force-pushed
the
speedup_isin
branch
from
7387b6d
to
7d706da
Compare
|
The timings are (pls use When the look-up is dominated by the calculation of the hash-function (small numbers), we see the disadvantages of #36729 - it is costlier now (almost factor 3). However, already for n about 100 we see the advantages of a more robust hash-function: for some series we are almost 10 times faster (e.g. The question is: is it worth to keep Seeing the numbers, I would say "Yes", even if it make the code harder to understand/maintain. What is your opinion @jreback @jbrockmendel @WillAyd ? |
|
yeah the question here is there is a good benefit for hashing when you have x values to lookup but if fewer then the constant time creation hits you so not averse to having a switchover point for a perf reasons at the expense of complexity |
|
The maintenance burden for this optimization is pretty small, I'm OK with keeping the switchover I think the bigger optimization available (that would also decrease the [non-cython] complexity) is getting hashtables with more dtypes so we dont have to cast to int64 xref #33287 |
|
@realead ping when ready here |
jreback
approved these changes
Nov 17, 2020
|
thanks @realead very nice! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffnumpy's
in1duses look-up inO(log n)inin1dcompared to panda'sO(1)so for a large number of unique elements invaluespandas solution will become better.Here is comparison in the performance benchmark:
It looks as if for more than 10
values-elements, panda's approach is faster.See also this comment (#22205 (comment)).
The question is whether it really makes sense to keep numpy's approach for less than 10
values-elements.