-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF: Performance improvement value_counts for masked arrays #48338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
self.data = np.array(list(range(1_000_000))) | ||
|
||
def time_constructor(self): | ||
Series(data=self.data, dtype="Int64") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have a benchmark for this in array.py (IntegerArray::time_from_integer_array), except that that one is with a tiny array and thus won't cover this aspect. But maybe add a version with a larger array in the existing benchmark? (or just make the array in that benchmark bigger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, forgot to check for series after I found nothing for value_counts. Increased the array size for the existing benchmark. 4 values is probably a bit small to see anything with overhead from other calls that don't depend on array size
@@ -166,6 +175,19 @@ def time_value_counts(self, N, dtype): | |||
self.s.value_counts() | |||
|
|||
|
|||
class ValueCountsEa: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class ValueCountsEa: | |
class ValueCountsEA: |
? Small nitpick: not sure if we have some consistency around this, but if not, since we typically use EA as abbreviation, would keep it capitalized here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, will keep in mind for future prs
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.cc @jorisvandenbossche