Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define promote_rule for Diagonal matrices #42142

Merged
merged 5 commits into from
Nov 12, 2021

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Sep 7, 2021

This PR adds a promote_rule definition for Diagonal matrices using the promotion rule for the parent diagonal vectors. I've also added a method Diagonal{T, V}(::Diagonal) to carry out necessary conversions. This differs from the constructor Diagonal{T,V}(::AbstractVector), and I hope that the usage is clear from the context. Currently the conversion throws an error, so this is not a breaking change.

This has two positives:

bugfix

Master

julia> D = Diagonal(1:3)
3×3 Diagonal{Int64, UnitRange{Int64}}:
 1    
   2  
     3

julia> D^0
ERROR: MethodError: no method matching Vector{Int64}(::Diagonal{Int64, UnitRange{Int64}})
Closest candidates are:
  Array{T, N}(::AbstractArray{S, N}) where {T, N, S} at array.jl:540
  Vector{T}() where T at boot.jl:467
  Array{T, N}(::StaticArrays.SizedArray{S, T, N, M, TData} where {M, TData<:AbstractArray{T, M}}) where {T, S, N} at /home/jb6888/.julia/packages/StaticArrays/vxjOO/src/SizedArray.jl:98
  ...
Stacktrace:
 [1] convert(#unused#::Type{Vector{Int64}}, a::Diagonal{Int64, UnitRange{Int64}})
   @ Base ./array.jl:532
 [2] Diagonal{Int64, Vector{Int64}}(diag::Diagonal{Int64, UnitRange{Int64}})
   @ LinearAlgebra /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/diagonal.jl:10
 [3] convert(T::Type{Diagonal{Int64, Vector{Int64}}}, m::Diagonal{Int64, UnitRange{Int64}})
   @ LinearAlgebra /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/special.jl:53
 [4] to_power_type(x::Diagonal{Int64, UnitRange{Int64}})
   @ Base ./intfuncs.jl:241
 [5] power_by_squaring(x_::Diagonal{Int64, UnitRange{Int64}}, p::Int64)
   @ Base ./intfuncs.jl:256
 [6] ^(A::Diagonal{Int64, UnitRange{Int64}}, p::Int64)
   @ LinearAlgebra /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/dense.jl:447
 [7] macro expansion
   @ ./none:0 [inlined]
 [8] literal_pow(f::typeof(^), x::Diagonal{Int64, UnitRange{Int64}}, #unused#::Val{0})
   @ Base ./none:0
 [9] top-level scope
   @ REPL[298]:1

This PR

julia> D^0
3×3 Diagonal{Int64, Vector{Int64}}:
 1    
   1  
     1

type-stability

Currently on master

julia> [Diagonal([1]), Diagonal([1.0])]
2-element Vector{Diagonal}:
 [1;;]
 [1.0;;]

julia> [Diagonal([ones(Int,1,1)]), Diagonal([ones(1,1)])]
2-element Vector{Diagonal}:
 [[1;;];;]
 [[1.0;;];;]

The element types are not concrete, as promotion falls back to typejoin here.

After this PR

ulia> [Diagonal([1]), Diagonal([1.0])]
2-element Vector{Diagonal{Float64, Vector{Float64}}}:
 [1.0;;]
 [1.0;;]

julia> [Diagonal([ones(Int,1,1)]), Diagonal([ones(1,1)])]
2-element Vector{Diagonal{Matrix{Float64}, Vector{Matrix{Float64}}}}:
 [[1.0;;];;]
 [[1.0;;];;]

@jishnub jishnub marked this pull request as draft September 7, 2021 12:45
@jishnub jishnub marked this pull request as ready for review September 7, 2021 14:48
@jishnub
Copy link
Contributor Author

jishnub commented Sep 12, 2021

@dkarrasch would like your opinion on this

@dkarrasch dkarrasch added the linear algebra Linear algebra label Sep 12, 2021
@dkarrasch
Copy link
Member

@timholy Would you be able to take another quick look? @jishnub I haven't touched the tests, so this should still do what you wanted it to do?

@dkarrasch dkarrasch merged commit 3ef1f61 into JuliaLang:master Nov 12, 2021
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@jishnub jishnub deleted the promotediagonal branch August 17, 2022 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants