JuliaArrays / ReadOnlyArrays.jl

A wrapper type around AbstractArray that is read-only
Other
27 stars 5 forks source link

Bug: `copy` and `similar` should return parent array type, not ReadOnlyArray #11

Closed MilesCranmer closed 3 months ago

MilesCranmer commented 3 months ago

@bkamins @markmbaum

There was inadvertently introduced in #9. It hasn't yet been released though.

On 0.1, ReadOnlyArray has the following behavior:

julia> X = ReadOnlyArray([1, 2])
2-element ReadOnlyArray{Int64, 1, Vector{Int64}}:
 1
 2

julia> c = copy(X)
2-element Vector{Int64}:
 1
 2

julia> c .= c .^ 2
2-element Vector{Int64}:
 1
 4

Which makes ReadOnlyArrays compatible with many libraries that create buffers from the input array. You would store your input data in a ReadOnlyArray, and expect the underlying package to make copies of it and then operate on the copy.

similar works by creating a copy of the parent array, rather than another ReadOnlyArray.

However, after 0.1, this is broken:

julia> X = ReadOnlyArray([1, 2])
2-element ReadOnlyVector{Int64, Vector{Int64}}:
 1
 2

julia> similar(X)
2-element ReadOnlyVector{Int64, Vector{Int64}}:
 0
 0

similar does not make sense as an operation unless you can mutate the elements, so I think this was introduced by mistake in #9.

For example, this means that core operations like copy will just break:

julia> c = copy(X)
ERROR: CanonicalIndexError: setindex! not defined for ReadOnlyVector{Int64, Vector{Int64}}
Stacktrace:
 [1] error_if_canonical_setindex(::IndexLinear, A::ReadOnlyVector{Int64, Vector{Int64}}, ::Int64)
   @ Base ./abstractarray.jl:1404
 [2] setindex!
   @ ./abstractarray.jl:1395 [inlined]
 [3] copyto_unaliased!
   @ ./abstractarray.jl:1088 [inlined]
 [4] copyto!(dest::ReadOnlyVector{Int64, Vector{Int64}}, src::ReadOnlyVector{Int64, Vector{Int64}})
   @ Base ./abstractarray.jl:1068
 [5] copymutable(a::ReadOnlyVector{Int64, Vector{Int64}})
   @ Base ./abstractarray.jl:1202
 [6] copy(a::ReadOnlyVector{Int64, Vector{Int64}})
   @ Base ./abstractarray.jl:1145
 [7] top-level scope
   @ REPL[5]:1

I think this should be reversed to the behavior of 0.1.

markmbaum commented 3 months ago

Thanks for fixing @bkamins. Lost track of this.