Skip to content
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

CLN/PERF: Simplify argmin/argmax #58019

Merged
merged 5 commits into from
Apr 1, 2024

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach commented Mar 26, 2024

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Ref: #57971 (comment)

ASVs against main:

| Change   | Before [b63ae8c7]    | After [46bbd5eb] <cln_argmin_argmax>   |   Ratio | Benchmark (Parameter)                                         |
|----------|----------------------|----------------------------------------|---------|---------------------------------------------------------------|
| -        | 184±1μs              | 138±3μs                                |    0.75 | series_methods.NanOps.time_func('argmax', 1000000, 'int32')   |
| -        | 14.4±0.1μs           | 10.5±0.2μs                             |    0.73 | series_methods.NanOps.time_func('argmax', 1000, 'float64')    |
| -        | 1.23±0.06ms          | 863±20μs                               |    0.7  | series_methods.NanOps.time_func('argmax', 1000000, 'float64') |
| -        | 77.9±0.6μs           | 40.8±0.7μs                             |    0.52 | series_methods.NanOps.time_func('argmax', 1000000, 'int8')    |
| -        | 7.42±0.2μs           | 2.44±0.06μs                            |    0.33 | series_methods.NanOps.time_func('argmax', 1000, 'int64')      |
| -        | 7.18±0.2μs           | 2.29±0.02μs                            |    0.32 | series_methods.NanOps.time_func('argmax', 1000, 'int32')      |
| -        | 7.25±0.1μs           | 2.29±0.03μs                            |    0.32 | series_methods.NanOps.time_func('argmax', 1000, 'int8')       |

ASVs against 2.2.x show no perf change.

@rhshadrach rhshadrach added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Performance Memory or execution speed performance Clean Reduction Operations sum, mean, min, max, etc. labels Mar 26, 2024
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks really nice

pandas/core/indexes/base.py Outdated Show resolved Hide resolved
@@ -1066,7 +1066,7 @@ def test_idxmin(self, float_frame, int_frame, skipna, axis):
frame.iloc[15:20, -2:] = np.nan
for df in [frame, int_frame]:
if (not skipna or axis == 1) and df is not int_frame:
if axis == 1:
if skipna:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we still be hitting this with the axis == 1 case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When axis==1 we run into all NAs for a single row, but when skipna=False we still raise the error message "Encountered an NA value with skipna=False" for performance - see the bottom half of #57971 (comment)

@@ -733,12 +733,6 @@ def argmax(
"""
delegate = self._values
Copy link
Member Author

@rhshadrach rhshadrach Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without using the delegate here and the nanops.nanargmax branch below, I'm seeing a 50x perf regression in some ASVs, e.g. series_methods.NanOps.time_func('argmax', 1000000, 'int8'):

N = 1000000
dtype = 'int8'
s = Series(np.ones(N), dtype=dtype)
s.argmax()

@jbrockmendel - perhaps that branch should be deeper in the code where the argmin/argmax implementation is for EAs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean if you do return return self.array.argmax(skipna=skipna) without the EA check?

It looks like sorting.nanargminmax is just a lot slower than the nanops.arg(min|max). Best case would be to improve sorting.nanargminmax, but failing that I think making NumpyExtensionArray.argmax call the faster version would be viable (need to double-check they are behaviorally equivalent)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean if you do return return self.array.argmax(skipna=skipna) without the EA check?

Correct.

Best case would be to improve sorting.nanargminmax, but failing that I think making NumpyExtensionArray.argmax call the faster version would be viable

Thanks - will give these a shot.

Copy link
Member Author

@rhshadrach rhshadrach Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this, and I don't think either is viable. The main reason nanops.nanargmax is faster than sorting.nanargminmax is that the former only works for numeric dtypes by filling in NA values where appropriate (typically with -np.inf) whereas for EAs (e.g. StringArray) we need to perform indexing operations to remove NA values from any consideration since there is no equivalent of -np.inf I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bummer. thanks for takinga look

@rhshadrach rhshadrach marked this pull request as ready for review March 30, 2024 13:14
@mroeschke mroeschke added this to the 3.0 milestone Apr 1, 2024
@mroeschke mroeschke merged commit aad1136 into pandas-dev:main Apr 1, 2024
46 checks passed
@mroeschke
Copy link
Member

Thanks @rhshadrach

@rhshadrach rhshadrach deleted the cln_argmin_argmax branch April 2, 2024 02:19
pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024
* CLN/PERF: Simplify argmin/argmax

* More simplifications

* Partial revert

* Remove comments

* fixups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Performance Memory or execution speed performance Reduction Operations sum, mean, min, max, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants