Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

MAINT: strides comparison performance fix, compare discussion #29179 #29188

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 1 commit into from
Jun 18, 2025

Conversation

xor2k
Copy link
Contributor

@xor2k xor2k commented Jun 12, 2025

As discussed in #29179, here a strides comparison performance fix: negative strides were not accounted for which could have led to lower-than-otherwise-possible performance. Now absolute stride values are considered instead.

@mattip
Copy link
Member

mattip commented Jun 12, 2025

Did any benchmarks change?

@xor2k
Copy link
Contributor Author

xor2k commented Jun 12, 2025

There are no benchmarks for this yet. However, I will modify the one I created last time so we get some numbers on that, too as soon as I have a free moment.

@seberg
Copy link
Member

seberg commented Jun 13, 2025

Might be nice to see a small change in benchmarks. But if it doesn't change much at all, I don't mind. It's just better to use the correct logic either way, IMO.
That said, labs is incorrect on windows since long is 32bits, may need to actually use if/else (I guess with a macro helper). Not sure we already have a macro, it doesn't really look like it.

@xor2k
Copy link
Contributor Author

xor2k commented Jun 15, 2025

Figured out some old fashioned ABS macro will do the trick, can't be more portable 😅

Regarding benchmarks: the matmul should take longer than the copying, but I assume one could create some particularly weird slice to make things measureable. What do you think?

@seberg seberg changed the title strides comparison performance fix, compare discussion #29179 MAINT: strides comparison performance fix, compare discussion #29179 Jun 18, 2025
@seberg
Copy link
Member

seberg commented Jun 18, 2025

I'll just put this in. I think it is unclear that this helps as discussed. But if it doesn't help, chances are we should just never bother doing a transposed copy (because the matmul is much slower anyway).
But, I think it's weir dto use the right transposed copy only for positive strides :).

@seberg seberg merged commit d52b36e into numpy:main Jun 18, 2025
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.