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

Specialize map for eltype conversions of a ReshapedArray #40678

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented May 1, 2021

Previously,

julia> map(Float64, reshape(1:4, 2, 2))
2×2 Matrix{Float64}:
 1.0  3.0
 2.0  4.0

After this PR:

julia> map(Float64, reshape(1:4, 2, 2))
2×2 reshape(::StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}}, 2, 2) with eltype Float64:
 1.0  3.0
 2.0  4.0

The map is translated to the parent, which may use specialized methods defined for the parent. This might reduce allocations.

Previously

julia> @btime map(Float64, $(reshape(1:400, 20, 20)));
  1.054 μs (1 allocation: 3.25 KiB)

After this PR:

julia> @btime map(Float64, $(reshape(1:400, 20, 20)));
  264.575 ns (0 allocations: 0 bytes)

base/reshapedarray.jl Outdated Show resolved Hide resolved
@ViralBShah
Copy link
Member

Merge?

@jishnub
Copy link
Contributor Author

jishnub commented Mar 9, 2024

The doctest that fails:

julia> x = reshape(1:12, 2, 3, 2)
2×3×2 reshape(::UnitRange{Int64}, 2, 3, 2) with eltype Int64:
[:, :, 1] =
 1  3  5
 2  4  6

[:, :, 2] =
 7   9  11
 8  10  12

julia> mask = map(ispow2, x)
2×3×2 reshape(::Vector{Bool}, 2, 3, 2) with eltype Bool:
[:, :, 1] =
 1  0  0
 1  1  0

[:, :, 2] =
 0  0  0
 1  0  0

whereas this used to be an Array on master. While this PR specifically introduced this change, I wonder if this will have performance impacts downstream? Some basic testing indicates a minor regression:

julia> @btime $x[$mask];
  742.000 ns (2 allocations: 144 bytes) # master
  888.705 ns (2 allocations: 144 bytes) # PR

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.

4 participants