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

more powerful reverse(A; dims) #37367

Merged
merged 15 commits into from
Sep 5, 2020
Merged

more powerful reverse(A; dims) #37367

merged 15 commits into from
Sep 5, 2020

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Sep 3, 2020

Fixes #37358:

  • Supports reversing multiple dimensions simultaneously in reverse(A; dims) by passing a tuple for dims
  • Makes dims=: (reverse all dimensions) the default for multidimensional A. (Previously it was an error not to specify dims) — similar to numpy.flip.
  • Adds multidimensional reverse!(A; dims)
  • Consolidates the Array and AbstractArray implementations
  • Seems to be mostly faster than the old reverse(A; dims) for flipping a single dimension.

Less code, does more, faster, backwards compatible — what's not to like?

To do:

  • Fix OffsetArrays test failures.
  • More tests
  • News

@stevengj stevengj added the arrays [a, r, r, a, y, s] label Sep 3, 2020
@martinholters martinholters added the needs compat annotation Add !!! compat "Julia x.y" to the docstring label Sep 3, 2020
@stevengj stevengj removed the needs compat annotation Add !!! compat "Julia x.y" to the docstring label Sep 3, 2020
@stevengj
Copy link
Member Author

stevengj commented Sep 3, 2020

(There are certainly ways to speed this up further. Working out of place, for example, just doing r(X::AbstractMatrix) = X[reverse(axes(X,1)), reverse(axes(X,2))] is a couple times faster than reverse(X). But I'm not sure how much effort it is worth to performance-optimize reverse.)

@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2020

analyzegc test failure seems unrelated; should be good to merge.

@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2020

PS. The inspiration for this PR was watching @3b1b doing awkward nested reverse calls in his 18.S191 lecture 1.

@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2020

A quick benchmark: for A = rand(1026,2102), doing reverse(reverse(A, dims=1), dims=2) with Julia 1.5 takes 228ms, doing the same call with the new version takes 11ms, doing reverse(A) (which flips both dimensions at once) takes 5.9ms, and doing reverse!(A) takes 3ms.

numpy.flip actually returns a view (analogous to view(A, reverse.(axes(A))...) in Julia), so the analogue of reverse(A) is numpy.flip(A).copy(). With the same matrix (which is big enough that Python overhead, which is < 1µs, is negligible) this takes 15ms on my machine.

@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2020

A thought: should something like @view reverse(A) or @views reverse(A) change it to return a view?

@stevengj stevengj merged commit b365157 into master Sep 5, 2020
@stevengj stevengj deleted the sgj/reversedims branch September 5, 2020 13:16
@stevengj stevengj mentioned this pull request Sep 5, 2020
@musm musm mentioned this pull request Dec 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support multiple dimensions in reverse(A, dims=...)
3 participants