Skip to content

PERF: only apply nanops rowwise optimization for narrow arrows #44566

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

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Nov 21, 2021

See the timings in #43311 (comment)

(this is a regression compared to previous release mostly when using ArrayManager (and thus doesn't need a whatsnew) and is covered by the FrameOps benchmarks)

@jorisvandenbossche jorisvandenbossche added the Performance Memory or execution speed performance label Nov 21, 2021
@jorisvandenbossche jorisvandenbossche added this to the 1.4 milestone Nov 21, 2021
@@ -463,6 +463,7 @@ def newfunc(values: np.ndarray, *, axis: int | None = None, **kwargs):
axis == 1
and values.ndim == 2
and values.flags["C_CONTIGUOUS"]
and (values.shape[1] / values.shape[0]) > 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some comments about why these things are needed (e.g. for performance), can ref this issue as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a comment with reference to my timings and update the docstring

@jorisvandenbossche
Copy link
Member Author

Verified that the remaining failure is only an unrelated ResourceWarning

@jreback jreback merged commit d5f7723 into pandas-dev:master Nov 23, 2021
@jreback
Copy link
Contributor

jreback commented Nov 23, 2021

thanks @jorisvandenbossche

@jorisvandenbossche jorisvandenbossche deleted the am-perf-nanops branch November 23, 2021 15:56
@jbrockmendel
Copy link
Member

LGTM. merits an asv?

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Nov 24, 2021

LGTM. merits an asv?

It gave a huge slowdown in the current ASVs when running with ArrayManager, so in that way it's already captured.

We could add one for wide dataframes as well, but you already need an extreme shape (like 10, 1000), so I personally don't find that a very interesting one to add. Misremembered this, so you actually already see a speed-up with something like (1000, 10).

@jbrockmendel
Copy link
Member

so in that way it's already captured

id be more comfortable with one that always runs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants