Preserve axes in permutedims for AbstractVectors#243
Preserve axes in permutedims for AbstractVectors#243jishnub merged 4 commits intoJuliaArrays:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #243 +/- ##
==========================================
+ Coverage 95.39% 95.47% +0.07%
==========================================
Files 5 5
Lines 413 420 +7
==========================================
+ Hits 394 401 +7
Misses 19 19
Continue to review full report at Codecov.
|
| @testset "permutedims" begin | ||
| a = OffsetArray(1:2, 2:3) | ||
| @test permutedims(a) == reshape(1:2, 1, 2:3) | ||
| a = OffsetArray([10,11], Base.OneTo(2)) | ||
| @test permutedims(a) == reshape(10:11, 1, 1:2) | ||
| a = OffsetArray(SVector{2}(1,2), 3:4) | ||
| @test permutedims(a) == reshape(1:2, 1, 3:4) | ||
| end |
There was a problem hiding this comment.
Maybe add 2d array tests to make sure the behavior is consistent?
There was a problem hiding this comment.
Sure, but those should be unaffected as the method is only added for OffsetVectors
| Base.reshape(A::OffsetArray, inds::Union{Int,Colon}...) = reshape(parent(A), inds) | ||
| Base.reshape(A::OffsetArray, inds::Tuple{Vararg{Union{Int,Colon}}}) = reshape(parent(A), inds) | ||
|
|
||
| # permutedims in Base does not preserve axes, and can not be fixed in a non-breaking way |
There was a problem hiding this comment.
Would you explain it a bit why this can't be fixed in a non-breaking way? The following version looks good to me.
julia> using OffsetArrays
julia> function Base.permutedims(v::AbstractVector)
out = similar(v, (1, axes(v, 1)))
copyto!(out, v)
end
julia> permutedims(rand(4))
1×4 Matrix{Float64}:
0.062106 0.864693 0.0235159 0.929236
julia> x = OffsetArray(rand(4,), -1);
julia> permutedims(x)
1×4 OffsetArray(::Matrix{Float64}, 1:1, 0:3) with eltype Float64 with indices 1:1×0:3:
0.449456 0.0518755 0.689773 0.00949295There was a problem hiding this comment.
The docstring for permutedims states that it must be a reshape for AbstractVectors (that is the underlying data is shared), however reshape does not currently accept custom axis types as arguments.
Although I might be misinterpreting what the docstring tries to say. Perhaps it uses reshape loosely.
There was a problem hiding this comment.
JuliaLang/julia#41003 is a good but controversial call; I don't think there will be a generic fix until #87 (comment) is solved; there will be many similar issues as identified by @mcabbott.
This specialization itself makes sense without being involved in type piracy so there's no objection in adding this to OffsetArrays.
|
8409f01 pops the parent array in a Previously: julia> reshape(zeros(6), Int32(2), :)
2×3 OffsetArray(::Matrix{Float64}, 1:2, 1:3) with eltype Float64 with indices 1:2×1:3:
0.0 0.0 0.0
0.0 0.0 0.0After this PR: julia> reshape(zeros(6), Int32(2), :)
2×3 Matrix{Float64}:
0.0 0.0 0.0
0.0 0.0 0.0 |
|
@johnnychen94 good to merge? |
johnnychen94
left a comment
johnnychen94
left a comment
There was a problem hiding this comment.
Efforts to minimize the effect due to type piracy is appreciated.
2 participants
The
Baseimplementation ofpermutedimsdoes not preserve axes forAbstractVectorarguments. This is a temporary solution to the problem forOffsetArrays, as the problem is likely to arise for these.Not sure if a general solution may be added to
Base, asreshapefor custom axis types does not exist as a concept currently. Perhaps something like this may be added in the longer term toBaseand this method may be version-limited.On master:
After this PR: