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

Converting a vector to Diagonal shouldn't work #26178

Closed
dlfivefifty opened this issue Feb 23, 2018 · 7 comments · Fixed by #26330
Closed

Converting a vector to Diagonal shouldn't work #26178

dlfivefifty opened this issue Feb 23, 2018 · 7 comments · Fixed by #26330
Assignees
Labels
arrays [a, r, r, a, y, s]
Milestone

Comments

@dlfivefifty
Copy link
Contributor

This new behaviour seems wrong:

julia> convert(Diagonal, rand(5))
5×5 Diagonal{Float64,Array{Float64,1}}:
 0.731736                                    
          0.501643                           
                   0.520562                  
                            0.390789         
                                     0.237735

I think the old behaviour of only supporting Diagonal(rand(5)) was correct.

@fredrikekre
Copy link
Member

I guess

convert(::Type{T}, a::AbstractArray) where {T<:AbstractArray} = T(a)

should be restricted w.r.t the dimension of the array? Something like

diff --git a/base/abstractarray.jl b/base/abstractarray.jl
index ff81756..1077add 100644
--- a/base/abstractarray.jl
+++ b/base/abstractarray.jl
@@ -12,7 +12,7 @@ Supertype for `N`-dimensional arrays (or array-like types) with elements of type
 AbstractArray
 
 convert(::Type{T}, a::T) where {T<:AbstractArray} = a
-convert(::Type{T}, a::AbstractArray) where {T<:AbstractArray} = T(a)
+convert(::Type{T}, a::AbstractArray{<:Any,N}) where {N,T<:AbstractArray{<:Any,N}} = T(a)
 
 if nameof(@__MODULE__) === :Base  # avoid method overwrite
 # catch undefined constructors before the deprecation kicks in

?

@dlfivefifty
Copy link
Contributor Author

I think that line should probably be removed completely, by the same logic that it was removed for general types.

@mbauman mbauman added the arrays [a, r, r, a, y, s] label Feb 23, 2018
@mbauman
Copy link
Member

mbauman commented Feb 23, 2018

This is a problem. We've been slowly moving to a world that supports T(a) where T <: AbstractArray and a isa AbstractArray to construct a new instance of T with the contents of a. LinearAlgebra's structured matrices do something totally different.

@Sacha0
Copy link
Member

Sacha0 commented Feb 23, 2018

Some discussion in #24595 :).

@dlfivefifty
Copy link
Contributor Author

I was involved in that discussion and I don't think there's another example in Base as egregiously confusing as Diagonal([1,2,3]).

One solution (which I used in BandedMatrices.jl) is to add an underscore _Diagonal([1,2,3]), but this should only be an internal solution.

This could be combined with an exported (and un-deprecated) diagm, so that diagm([1,2,3]) returns a Diagonal matrix.

@mbauman mbauman added the triage This should be discussed on a triage call label Mar 1, 2018
@JeffBezanson
Copy link
Member

We should narrow the convert definition only to cases that do the right thing (i.e. the input and output are sufficiently similar).

@mbauman
Copy link
Member

mbauman commented Mar 1, 2018

And we shouldn't require 1-argument constructors for AbstractArrays to construct a new array based upon the iteration of the argument filling all cartesian values of the array.

@mbauman mbauman removed the triage This should be discussed on a triage call label Mar 1, 2018
@mbauman mbauman added this to the 1.0 milestone Mar 1, 2018
@JeffBezanson JeffBezanson self-assigned this Mar 5, 2018
JeffBezanson added a commit that referenced this issue Mar 5, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 6, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 6, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 7, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
JeffBezanson added a commit that referenced this issue Mar 8, 2018
Not all array types can convert from any AbstractArray via a
1-argument constructor call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants