Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Feb 20, 2021

There are a couple of changes that I have removed from other PRs, as they are potentially breaking (leads to changes in return types). We may consider these for version 2.0 (or perhaps a 1.x minor release if these are not considered breaking).

  1. copy is forwarded to the parent. This preserves the type of the parent in case there is a special structure associated with it. For example:

On master:

julia> a = [1,2]';

julia> ao = OffsetArray(a, axes(a));

julia> copy(a) # produces an Adjoint
1×2 Adjoint{Int64,Array{Int64,1}}:
 1  2

julia> copy(ao) # produces a 1-row matrix with offset axes
1×2 OffsetArray(::Array{Int64,2}, 1:1, 1:2) with eltype Int64 with indices 1:1×1:2:
 1  2

The copy operation on the Adjoint of an AbstractVector preserves its type, as a row-vector is distinct from a 1-row matrix. However OffsetArrays do not preserve this type information. Also for immutable parent structs such as AbstarctRanges and StaticArrays, the copy appears to not respect the parent's behavior:

julia> s = @SVector[i for i in 1:3]; so = OffsetArray(s, 3); # StaticArray

julia> copy(s) # produces an SArray
3-element SArray{Tuple{3},Int64,1,3} with indices SOneTo(3):
 1
 2
 3

julia> copy(so) # no longer static
3-element OffsetArray(::Array{Int64,1}, 4:6) with eltype Int64 with indices 4:6:
 1
 2
 3

julia> f = Fill(3, 2, 2); fo = OffsetArray(f, axes(f)); # FillArray

julia> copy(f) # produces a FillArray
2×2 Fill{Int64}: entries equal to 3

julia> copy(fo) # produces a dense array
2×2 OffsetArray(::Array{Int64,2}, 1:2, 1:2) with eltype Int64 with indices 1:2×1:2:
 3  3
 3  3

julia> r = 1:3; ro = OffsetArray(r, 2);

julia> copy(r) # produces a UnitRange
1:3

julia> copy(ro) # produces a dense array
3-element OffsetArray(::Array{Int64,1}, 3:5) with eltype Int64 with indices 3:5:
 1
 2
 3

After this PR:

julia> copy(ao)
1×2 OffsetArray(::Adjoint{Int64,Array{Int64,1}}, 1:1, 1:2) with eltype Int64 with indices 1:1×1:2:
 1  2

julia> copy(so)
3-element OffsetArray(::SArray{Tuple{3},Int64,1,3}, 4:6) with eltype Int64 with indices 4:6:
 1
 2
 3

julia> copy(fo)
2×2 OffsetArray(::Fill{Int64,2,Tuple{Base.OneTo{Int64},Base.OneTo{Int64}}}, 1:2, 1:2) with eltype Int64 with indices 1:2×1:2:
 3  3
 3  3

julia> copy(ro)
1:3 with indices 3:5
  1. getindex with as many Colons as the number of dimensions may preserve the type of the parent. This operation is similar to copy, and in the case of AbstactRanges it's actually implemented as as a copy.
julia> ao[:,:]
1×2 OffsetArray(::Adjoint{Int64,Array{Int64,1}}, 1:1, 1:2) with eltype Int64 with indices 1:1×1:2:
 1  2

julia> so[:]
3-element OffsetArray(::SArray{Tuple{3},Int64,1,3}, 4:6) with eltype Int64 with indices 4:6:
 1
 2
 3

@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

Merging #207 (1ec64a8) into master (ac9821e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   98.33%   98.36%   +0.02%     
==========================================
  Files           5        5              
  Lines         301      305       +4     
==========================================
+ Hits          296      300       +4     
  Misses          5        5              
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.03% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac9821e...1ec64a8. Read the comment docs.

@jishnub
Copy link
Member Author

jishnub commented Mar 1, 2021

Before:

julia> s = SArray{Tuple{2,2,2},Int,3,8}((1,2,3,4,5,6,7,8))
2×2×2 SArray{Tuple{2,2,2},Int64,3,8} with indices SOneTo(2)×SOneTo(2)×SOneTo(2):
[:, :, 1] =
 1  3
 2  4

[:, :, 2] =
 5  7
 6  8

julia> so = OffsetArray(s, axes(s));

julia> so[:] # not static anymore
8-element Array{Int64,1}:
 1
 2
 3
 4
 5
 6
 7
 8

After 0155aae

julia> so[:] # still static
8-element SArray{Tuple{8},Int64,1,8} with indices SOneTo(8):
 1
 2
 3
 4
 5
 6
 7
 8

Also,

Before:

julia> a = ones(2000, 2000);

julia> ao = OffsetArray(a, 2, 2);

julia> @btime $a[:]; # uses fast contiguous indexing with unsafe_copyto!
  4.196 ms (2 allocations: 30.52 MiB)

julia> @btime $ao[:];
  7.010 ms (2 allocations: 30.52 MiB)

After 0155aae

julia> @btime $ao[:];
  4.151 ms (2 allocations: 30.52 MiB)

@inline function Base.getindex(A::OffsetArray{T,N}, I::Vararg{Int,N}) where {T,N}
@propagate_inbounds Base.getindex(A::OffsetArray{<:Any,0}) = A.parent[]

@inline function Base.getindex(A::OffsetArray{<:Any,N}, I::Vararg{Int,N}) where N
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this should be @propagate_inbounds too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it? The idea here was to carry out the bounds checking using the axes of the OffsetArray and not to propagate it to the parent.

@jishnub jishnub force-pushed the copy_getindexcolon branch from ca9914a to 1ec64a8 Compare March 6, 2021 08:31
@jishnub
Copy link
Member Author

jishnub commented Mar 6, 2021

Good to merge?

@jishnub jishnub merged commit b74ae55 into JuliaArrays:master Apr 11, 2021
@jishnub jishnub deleted the copy_getindexcolon branch April 11, 2021 05:23
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.

2 participants