Handle changed copy_oftype

[rikhuijzer]

This fixes the tests on Julia 1.8-beta1. This fixes the underlying problem of JuliaArrays/LazyArrays.jl#205:

For example, on Julia 1.8:

julia> Base.broadcasted(-, 1:5, Zeros{Int}(5))
5-element Vector{Int64}:
 1
 2
 3
 4
 5

which used to be

julia> Base.broadcasted(-, 1:5, Zeros{Int}(5))
1:5

This was caused in changes to LinearAlgebra.copy_oftype in JuliaLang/julia#40831. As far as I understand, the meaning of copy_oftype used to be wrong and is now correct. I've solved the problem here by calling copy directly instead of copy_oftype.

[rikhuijzer]

This PR assumes that the following behavior is correct (Julia 1.8-beta1):

julia> LinearAlgebra.copy_oftype(1:5, Int)
5-element Vector{Int64}:
 1
 2
 3
 4
 5

versus the old behavior (Julia 1.7.2)

julia> LinearAlgebra.copy_oftype(1:5, Int)
1:5

EDIT: Daniel Karrasch (the reviewer of JuliaLang/julia#40831) responded on Slack on a question of mine about whether this behavior is correct. He said the following:

It's hard to say whether it's "correct" or not, since it doesn't follow a widely agreed API, but is rather internal. copy_oftype is supposed to give a writable object now, that's why the vector. Since it's based on 2-arg similar , you can change the outcome by overloading 2-arg similar or copy_oftype for your own types.

[rikhuijzer]

To verify this PR, I've ran the tests at JuliaArrays/LazyArrays.jl#205 with this PR. On Julia 1.7.2 all tests pass and on Julia 1.8-beta1, only 2 tests fail. Those 2 failing tests are due to problems with copy_oftype in FillArrays. I'm working on a PR for that too.

@dlfivefifty What do you think?

[dlfivefifty]

No this won't due as we need to promote the type.

I think just copy over the old copy_oftype.

Really LinearAlgebra should call it mutablecopy_oftype as copy is not in general mutable.

[codecov]

Codecov Report

Merging #172 (3348c50) into master (9d3d1cc) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #172      +/-   ##
==========================================
+ Coverage   96.90%   96.94%   +0.03%     
==========================================
  Files           4        4              
  Lines         646      654       +8     
==========================================
+ Hits          626      634       +8     
  Misses         20       20              

Impacted Files	Coverage Δ
src/fillbroadcast.jl	97.67% <100.00%> (+0.13%)	⬆️
src/FillArrays.jl	95.53% <0.00%> (+0.01%)	⬆️

---

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 9d3d1cc...3348c50. Read the comment docs.

[daanhb]

Hmm, copy_oftype is internal to LinearAlgebra and it is somewhat surprising that you would need it here. If copy works here then I guess that's fine. If you want to preserve structure and mutability, but possibly change the eltype, then convert(AbstractArray{T}, myarray) may do the trick. That is what the old copy_oftype did in most cases. Or AbstractArray{T}(myarray).

[rikhuijzer]

Hmm, copy_oftype is internal to LinearAlgebra and it is somewhat surprising that you would need it here. If copy works here then I guess that's fine. If you want to preserve structure and mutability, but possibly change the eltype, then convert(AbstractArray{T}, myarray) may do the trick. That is what the old copy_oftype did in most cases. Or AbstractArray{T}(myarray).

In this case, I copied the copy from the method that was called in Julia 1.7.2 so maybe there the generality of convert(AbstractArray{T}, myarray) or AbstractArray{T}(myarray) isn't needed. In any case, I suspect that copy will be the quickest.

But well, you guys know more so just let me know what you prefer 👍

[rikhuijzer]

No this won't due as we need to promote the type.

I think just copy over the old copy_oftype.

Really LinearAlgebra should call it mutablecopy_oftype as copy is not in general mutable.

That's what I did. I copied the content of the method that was called.

[daanhb]

In this case, I copied the copy from the method that was called in Julia 1.7.2 so maybe there the generality of convert(AbstractArray{T}, myarray) or AbstractArray{T}(myarray) isn't needed. In any case, I suspect that copy will be the quickest.

But well, you guys know more so just let me know what you prefer 👍

As far as I'm concerned, whatever works :-) I'm just making suggestions to resolve any issues here caused by changes to copy_oftype...

[rikhuijzer]

With copy:

julia> (1:5) .+ Zeros(5)
1:5

This is the correct output here.

With AbstractArray{promote_type(T, V)}(a) and convert(AbstractArray{promote_type(T, V)}, a):

julia> (1:5) .+ Zeros(5)
5-element Vector{Float64}:
 1.0
 2.0
 3.0
 4.0
 5.0

which is the original problem and caused by

julia> UnitRange{Int} <: AbstractVector{Int}
true

julia> Vector{Int} <: AbstractVector{Int}
true

The method for Julia 1.7 for LinearAlgebra.copy_oftype(1:5, Int) is copy:

copy_oftype(A::AbstractArray{T}, ::Type{T}) where {T} = copy(A)

source

If there is some case where copy isn't suitable, it would be nice if we could come with such a counterexample and add a test for it

[dlfivefifty]

The correct output for that example is 1.0:5.0 since the type needs to be promoted

[rikhuijzer]

The correct output for that example is 1.0:5.0 since the type needs to be promoted

Thanks. I was wrong indeed.

I've experimented a bit more with AbstractArray{promote_type(T, V)}(a) and alternatives, and it seems that the only working fix is to copy the methods for copy_oftype from LinearAlgebra. This results in all tests passing. I've tested it on Julia 1.7.2 and Julia 1.8-beta1.

[daanhb]

Hmm, please don't solve it like this. (You are now copying an old internal function from LinearAlgebra with unclear semantics, without changing its name, while the function in LinearAlgebra has meanwhile changed.)

There is an underlying problem and it may be better to fix that. In your example, the problem is, I think, not with FillArrays or with copy_oftype, the problem is:

julia> convert(AbstractVector{Float64}, 1:5)
5-element Vector{Float64}:
 1.0
 2.0
 3.0
 4.0
 5.0

This is a result I would disagree with in principle, but some code out there relies on this conversion returning a mutable array. So it would be a breaking change to change it in Base and I didn't even raise the issue. (I did check for occurrences of copy_oftype in the wild also before changing it and there were very few, you are unlucky here!)

The choice in Base so far has been to support the syntax map(T, myarray) instead:

julia> map(Float64, 1:5)
1.0:1.0:5.0

Could you experiment with that? I don't think it will work for all structured matrices though, as they all have to support this syntax and I don't think they all do. On the other hand, convert(AbstractArray{T}, A) should work for all structured matrices, but not for ranges. Perhaps it is sufficient to special case ranges in your code, and use convert(AbstractArray{T}, A) otherwise?

Sorry for the hassle!

[daanhb]

Ideally, you no longer use copy_oftype anymore, and in the future the function will hopefully be gone from LinearAlgebra. It is only useful in certain cases to obtain a structured array with a different element type which is suitable for in-place factorization algorithms. It works for now, but there is nothing generic about it.

[rikhuijzer]

map works great

[dlfivefifty]

I'm worried this is a bit too much of a "pun". In particular, it requires types to overload map and so won't work if only copy or convert is overloaded, which is the case for the structured matrix packages.

This is my proposal, add the following function and call it:

_copy_oftype(A::AbstractArray{T,N}, ::Type{T}) where {T,N} = copy(A)
_copy_oftype(A::AbstractArray{T,N}, ::Type{S}) where {T,N,S} = convert(AbstractArray{S,N}, A)
_copy_oftype(A::AbstractRange{T}, ::Type{T}) where T = copy(A)
_copy_oftype(A::AbstractRange{T}, ::Type{S}) where {T,S} = map(S, A)

[dlfivefifty]

without changing its name, while the function in LinearAlgebra has meanwhile changed

I think the problem is the name of the new LinearAlgebra.copy_oftype should be renamed to LinearALgebra.copymutable_oftype, since copy on its own does not imply mutable. I'm planning to make a PR.

[daanhb]

without changing its name, while the function in LinearAlgebra has meanwhile changed

I think the problem is the name of the new LinearAlgebra.copy_oftype should be renamed to LinearALgebra.copymutable_oftype, since copy on its own does not imply mutable. I'm planning to make a PR.

Fair enough I think. It is indeed intended to be mutable.

[rikhuijzer]

By the way, can we use this same fix for ArrayLayouts.jl? Then, I'll make a PR there too

[dlfivefifty]

Probably. I would just call FillArrays._copy_oftype from there (I know relying on internals is not generally good, but in this case they are both my packages)

[daanhb]

Is it correct that it really just comes down to the behaviour of ranges after conversion, or am I missing something? Is there another example where the new copy_oftype does not do what you want here?
I'm fine with any solution here, I'm just wondering about possible side-effects of copy_oftype elsewhere that we're not aware of.