Make map! correctly handle differently-sized containers.#50647
Make map! correctly handle differently-sized containers.#50647
Conversation
|
@nanosoldier |
362067c to
e833d4c
Compare
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
e833d4c to
9aefacd
Compare
|
Looks like this exposes some other existing issues, e.g. with OffsetArrays: julia> map!(+, [0, 0, 0, 0], fill(0, -4:-1), fill(0, -4:-1), fill(0, -4:-1))
Int64[]
julia> map!(+, [0, 0, 0, 0], fill(0, -4:-1), fill(0, -4:-1))
4-element Vector{Int64}:
0
0
0
0 |
400d905 to
e2be645
Compare
e2be645 to
84acf71
Compare
|
@nanosoldier |
|
The changes in allocations here can be ignored, I think. They come from the additional bounds checks, but only when running with include("../../test/testhelpers/OffsetArrays.jl")
using Main.OffsetArrays
R = fill(0, -4:-1, 7:7, -6:-6)
B = OffsetArray(reshape(1:24, 4, 3, 2), -5, 6, -7)
@allocated maximum!(R, B)1264 bytes on master, 1248 on this PR, but 1408 bytes when forcing bounds checks. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
The package evaluation job you requested has completed - possible new issues were detected. |
|
was the attempt here successful? is there anything I can do to assist? e.g. particular benchmark regressions |
|
Sadly not, the performance regressions are real, but I didn't have the time to look into them. Feel free to take this up and create your own PR, if you want. |
|
Closing as abandoned, feel free to re-open if you'd like to pick it back up |
Closes #36235.
The
map!docstring specifies:However, we're neither boundschecking the destination container, nor using the smallest input collection:
This PR corrects that.
Will need some careful benchmarking, maybe add some
@inboundstomap!calls and/or outlining the throwing.FWIW, the performance of the example in #30624 (comment) is identical.