Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Apr 28, 2021

Fixes #171 partly

Now conversion works if the OffsetArray type is fully specified (doesn't work at present if it's left as a UnionAll works now).

julia> d = Diagonal([1,1,1])
3×3 Diagonal{Int64, Vector{Int64}}:
 1    
   1  
     1

julia> convert(OffsetMatrix{Float64, Matrix{Float64}}, d)
3×3 OffsetArray(::Matrix{Float64}, 1:3, 1:3) with eltype Float64 with indices 1:3×1:3:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> convert(OffsetMatrix{Float32, Matrix{Float32}}, d)
3×3 OffsetArray(::Matrix{Float32}, 1:3, 1:3) with eltype Float32 with indices 1:3×1:3:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

julia> convert(OffsetMatrix{Float32, Matrix{Float32}}, OffsetArray(d, 2, 2))
3×3 OffsetArray(::Matrix{Float32}, 3:5, 3:5) with eltype Float32 with indices 3:5×3:5:
 1.0  0.0  0.0
 0.0  1.0  0.0
 0.0  0.0  1.0

As a consequence this matrix vector product works now (used to error earlier):

julia> a = [MMatrix{2,2}(1:4) for i = 1:2];

julia> oa = [OffsetArray(ai, 0, 0) for ai in a];

julia> b = ones(2,2);

julia> b * oa
2-element Vector{OffsetMatrix{Float64, SizedMatrix{2, 2, Float64, 2, Matrix{Float64}}}}:
 [2.0 6.0; 4.0 8.0]
 [2.0 6.0; 4.0 8.0]

These constructors create copies if necessary.

I have not added the other UnionAll constructors deliberately as it'll not be clear if the OffsetArray(::AbstractArray) constructor creates a copy or not. Currently it doesn't create a copy for OffsetArray(::Array{T,0}), so creating a copy for other arrays might be confusing. On the other hand, we may choose to not create a copy at all, which will make the OffsetArray constructor behave differently from Array.

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #230 (7293ff6) into master (cca160c) will increase coverage by 0.38%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   94.69%   95.07%   +0.38%     
==========================================
  Files           5        5              
  Lines         358      386      +28     
==========================================
+ Hits          339      367      +28     
  Misses         19       19              
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.12% <96.87%> (-0.22%) ⬇️
src/utils.jl 100.00% <100.00%> (+3.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 cca160c...7293ff6. Read the comment docs.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

it'll not be clear if the OffsetArray(::AbstractArray) constructor creates a copy or not

We won't do deep copy; most constructors don't do it. Since offsets is a tuple, copy the wrapper type or not doesn't make much difference. My point is: OffsetArray(a::OffsetArray) === OffsetArray(a::OffsetArray) is not expected to hold in general. Users' codes should not depend on these internal differences.

@jishnub
Copy link
Member Author

jishnub commented Apr 28, 2021

I've added the extra constructors necessary, so now general conversions work.

julia> convert(OffsetVector, 1:4)
1:4 with indices 1:4

julia> convert(OffsetVector{Float32}, 1:4)
1.0f0:1.0f0:4.0f0 with indices 1:4

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Except for the small "bug", all others are good to me. (I like how you write tests 👍 )

@jishnub jishnub merged commit bff8e7b into JuliaArrays:master May 2, 2021
@jishnub jishnub deleted the convert branch May 2, 2021 08:12
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.

missing convert method

2 participants