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

full on an AbstractArray should not be a no-op #12251

Closed
jakebolewski opened this issue Jul 21, 2015 · 17 comments
Closed

full on an AbstractArray should not be a no-op #12251

jakebolewski opened this issue Jul 21, 2015 · 17 comments
Assignees
Labels
Milestone

Comments

@jakebolewski
Copy link
Member

The current fallback definition for full is a no-op for AbstractArray subtypes. @andreasnoack and I feel that this should be convert(Array, a) instead as the implicit contract for full is to return a dense array.

Another common subtype that does not densify the result are Range types:

julia> full(reverse(1:10))
10:-1:1
@JeffBezanson
Copy link
Sponsor Member

Unless full also promises a mutable array, leaving ranges alone might be ok.

Or, we could remove the fallback and only define full(a::Array).

@jakebolewski jakebolewski added this to the 0.5 milestone Jul 21, 2015
@mbauman
Copy link
Sponsor Member

mbauman commented Jul 22, 2015

I think full is a pretty crummy name, but I suppose it is the standard vocabulary for sparse arrays. convert(Array, x) seems to describe the operation quite well.

Alternatively, we could use dense(x) = convert(DenseArray, x), which may be a little more flexible but yet better defined.

@StefanKarpinski
Copy link
Sponsor Member

Can we just call this operation convert(Array, x) since that's what it does? Then if you want something less specific, we don't have to invent a name for it – just write convert(DenseArray, x), etc.

@tkelman
Copy link
Contributor

tkelman commented Jul 22, 2015

Agreed, ref #12153 for the other meaning we've semi-arbitrarily given to full. Maybe we just call this a matlab-ism and aim to get rid of it.

@StefanKarpinski
Copy link
Sponsor Member

Yes, I think it's badly and unintuitively named and not all that common an operation.

@jakebolewski
Copy link
Member Author

I would be happy to get rid of it. That decision falls in line with "having the user think about types" complaint that we get from time to time but honestly we are not Matlab and thinking about types is unavoidable.

@jakebolewski
Copy link
Member Author

Removing full causes a lot of excess allocation in the AbstractArray code for Range types.

@tkelman
Copy link
Contributor

tkelman commented Apr 21, 2016

Is this still 0.5 material? @andreasnoack?

@andreasnoack
Copy link
Member

andreasnoack commented Apr 21, 2016

I don't think this should block 0.5.

Many of the full(x)s could probably be changed to convert(Array, x) but we need to come up with a function for returning non-view version of a view, i.e. what copy does for SubArray but it should work for e.g. XTriangular and Symmetric and maybe also the factorizations. In these cases, we cannot use explicit conversion with a type because the "view" is parametric on the array type.

@tkelman tkelman removed this from the 0.5.0 milestone Apr 21, 2016
@tkelman
Copy link
Contributor

tkelman commented Apr 21, 2016

parent ?

@andreasnoack
Copy link
Member

I thought so too for a while but it doesn't do the right thing for SubArrays.

@tkelman
Copy link
Contributor

tkelman commented Apr 22, 2016

What are the use cases where you need a generic version of that operation that works over multiple views-of-AbstractArray types? You want a copy that includes the hidden parts of the backing data array?

@timholy
Copy link
Sponsor Member

timholy commented Apr 22, 2016

We already have a generic method for convert(Array, ::AbstractArray). So I don't think we're missing anything.

@tkelman
Copy link
Contributor

tkelman commented Apr 22, 2016

I think what Andreas is saying he wants is a generic way of making a copy of the underlying backing array. convert to Array on something like Triangular or Symmetric would have values that match what you would get from getindex, but the backing array will contain extra hidden values.

@tkelman
Copy link
Contributor

tkelman commented Apr 22, 2016

Or no, nevermind, scratch that. The method we don't have is using the type parameter of Symmetric or Triangular to determine the type of array to convert to.

@andreasnoack
Copy link
Member

The functions I had in mind were * and \ where we want a setindex!able version of one of the arguments. However, I cannot really come up with an example right now so maybe it's not that important. It will probably be a useful but potentially time-consuming effort to go through base and try to eliminate full.

andreasnoack pushed a commit that referenced this issue Jul 10, 2016
…nvert(Array, X) and add convert(AbstractArray, X) methods for Factorizations (#17066)

* Implement convert(Matrix, SparseMatrixCSC) <- convert(Array, SparseMatrixCSC) chain, and reimplement full(SparseMatrixCSC) as a synonymous child of convert(Array, SparseMatrixCSC).

* Implement convert(Vector, AbstractSparseVector) <- convert(Array, AbstractSparseVector) chain, and reimplement full(AbstractSparseVector) as a synonymous child of convert(Array, AbstractSparseVector).

* Implement convert(Matrix, Bidiagonal) <- convert(Array, Bidiagonal) chain, and reimplement full(Bidiagonal) as a synonymous child of convert(Array, Bidiagonal).

* Implement convert(AbstractMatrix, X) <- convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for Cholesky and CholeskyPivoted types, and reimplement full(X) for both types as synonymous children of convert(Array, X).

* Implement convert(Matrix, Diagonal) <- convert(Array, Diagonal) chain, and reimplement full(Diagonal) as a synonymous child of convert(Array, Diagonal).

* Implement convert(AbstractMatrix, Eigen) <- convert(AbstractArray, Eigen) <- convert(Matrix, Eigen) <- convert(Array, Eigen) chain, and reimplement full(Eigen) as synonymous child of convert(Array, Eigen).

* Implement convert(AbstractMatrix, Hessenberg) <- convert(AbstractArray, Hessenberg) <- convert(Matrix, Hessenberg) <- convert(Array, Hessenberg) and convert(Matrix, HessenbergQ) <- convert(Array, HessenbergQ )chains, and reimplement full(X) for both types as synonmous children of convert(Array, X).

* Implement convert(SymTridiagonal, LDLt) <- convert(AbstractMatrix, LDLt) <- convert(AbstractArray, LDLt) <- convert(Matrix, LDLt) <- convert(Array, LDLt) chain, and reimplement full(LDLt) as a synonymous child of convert(Array, LDLt).

* Implement convert(AbstractMatrix, LQ) <- convert(AbstractArray, LQ) <- convert(Matrix, LQ) <- convert(Array, LQ) and convert(Matrix, LQPackedQ) <- convert(Array, LQPackedQ) chains, and reimplement full(X) for both types as a synonymous child of convert(Array, X).

* Implement convert(AbstractMatrix, X) < convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for LU and LU{T,Tridiagonal{T}} types, and reimplement full(X) for both types as synonymous children of convert(Array, X).

* Implement convert(AbstractArray, Union{QR,QRCompactWY}) <- convert(AbstractMatrix, Union{QR,QRCompactWY}) <- convert(Matrix, Union{QR,QRCompactWY}) <- convert(Array, Union{QR,QRCompactWY}) and convert(Matrix, Union{QRPackedQ,QRCompactWYQ}) <- convert(Array, Union{QRPackedQ,QRCompactWYQ}) chains, and reimplement full(Union{QR,QRCompactWY}) and full(Union{QRPackedQ,QRCompactWYQ}) as synonymous children of convert(Array, Union{QR,QRCompactWY}) and convert(Array, Union{QRPackedQ,QRCompactWYQ}) respectively.

* Implement convert(AbstractMatrix, Schur) <- convert(AbstractArray, Schur) <- convert(Matrix, Schur) <- convert(Array, Schur) chain, and reimplement full(Schur) as a synonymous child of convert(Array, Schur).

* Implement convert(AbstractMatrix, SVD) <- convert(AbstractArray, SVD) <- convert(Matrix, SVD) <- convert(Array, SVD) chain, and reimplement full(SVD) as a synonymous child of convert(Array, SVD).

* Implements convert(Matrix, Symmetric) and convert(Matrix, Hermitian). Implements convert(Array, Union{Symmetric,Hermitian}) as a short child of the two preceding methods, and reimplements full(Union{Symmetric,Hermitian}) likewise.

* Implements convert(Array, AbstractTriangular) as a short child of convert(Matrix, AbstractTriangular), and reimplements full(AbstractTriangular) as a short child of convert(Array, AbstractTriangular).

* Implements convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) as synonymous children of convert(Matrix, Tridiagonal) and convert(Matrix, SymTridiagonal) respectively, and reimplements full(Tridiagonal) and full(SymTridiagonal) as short children of convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) respectively.
tkelman added a commit that referenced this issue Jul 21, 2016
Towards #12251 part 2, replace all full(X) calls with convert(Array, X) and migrate tests
tkelman added a commit that referenced this issue Jul 22, 2016
tkelman added a commit that referenced this issue Jul 22, 2016
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
…method defs. as convert(Array, X) and add convert(AbstractArray, X) methods for Factorizations (JuliaLang#17066)

* Implement convert(Matrix, SparseMatrixCSC) <- convert(Array, SparseMatrixCSC) chain, and reimplement full(SparseMatrixCSC) as a synonymous child of convert(Array, SparseMatrixCSC).

* Implement convert(Vector, AbstractSparseVector) <- convert(Array, AbstractSparseVector) chain, and reimplement full(AbstractSparseVector) as a synonymous child of convert(Array, AbstractSparseVector).

* Implement convert(Matrix, Bidiagonal) <- convert(Array, Bidiagonal) chain, and reimplement full(Bidiagonal) as a synonymous child of convert(Array, Bidiagonal).

* Implement convert(AbstractMatrix, X) <- convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for Cholesky and CholeskyPivoted types, and reimplement full(X) for both types as synonymous children of convert(Array, X).

* Implement convert(Matrix, Diagonal) <- convert(Array, Diagonal) chain, and reimplement full(Diagonal) as a synonymous child of convert(Array, Diagonal).

* Implement convert(AbstractMatrix, Eigen) <- convert(AbstractArray, Eigen) <- convert(Matrix, Eigen) <- convert(Array, Eigen) chain, and reimplement full(Eigen) as synonymous child of convert(Array, Eigen).

* Implement convert(AbstractMatrix, Hessenberg) <- convert(AbstractArray, Hessenberg) <- convert(Matrix, Hessenberg) <- convert(Array, Hessenberg) and convert(Matrix, HessenbergQ) <- convert(Array, HessenbergQ )chains, and reimplement full(X) for both types as synonmous children of convert(Array, X).

* Implement convert(SymTridiagonal, LDLt) <- convert(AbstractMatrix, LDLt) <- convert(AbstractArray, LDLt) <- convert(Matrix, LDLt) <- convert(Array, LDLt) chain, and reimplement full(LDLt) as a synonymous child of convert(Array, LDLt).

* Implement convert(AbstractMatrix, LQ) <- convert(AbstractArray, LQ) <- convert(Matrix, LQ) <- convert(Array, LQ) and convert(Matrix, LQPackedQ) <- convert(Array, LQPackedQ) chains, and reimplement full(X) for both types as a synonymous child of convert(Array, X).

* Implement convert(AbstractMatrix, X) < convert(AbstractArray, X) <- convert(Matrix, X) <- convert(Array, X) chains for LU and LU{T,Tridiagonal{T}} types, and reimplement full(X) for both types as synonymous children of convert(Array, X).

* Implement convert(AbstractArray, Union{QR,QRCompactWY}) <- convert(AbstractMatrix, Union{QR,QRCompactWY}) <- convert(Matrix, Union{QR,QRCompactWY}) <- convert(Array, Union{QR,QRCompactWY}) and convert(Matrix, Union{QRPackedQ,QRCompactWYQ}) <- convert(Array, Union{QRPackedQ,QRCompactWYQ}) chains, and reimplement full(Union{QR,QRCompactWY}) and full(Union{QRPackedQ,QRCompactWYQ}) as synonymous children of convert(Array, Union{QR,QRCompactWY}) and convert(Array, Union{QRPackedQ,QRCompactWYQ}) respectively.

* Implement convert(AbstractMatrix, Schur) <- convert(AbstractArray, Schur) <- convert(Matrix, Schur) <- convert(Array, Schur) chain, and reimplement full(Schur) as a synonymous child of convert(Array, Schur).

* Implement convert(AbstractMatrix, SVD) <- convert(AbstractArray, SVD) <- convert(Matrix, SVD) <- convert(Array, SVD) chain, and reimplement full(SVD) as a synonymous child of convert(Array, SVD).

* Implements convert(Matrix, Symmetric) and convert(Matrix, Hermitian). Implements convert(Array, Union{Symmetric,Hermitian}) as a short child of the two preceding methods, and reimplements full(Union{Symmetric,Hermitian}) likewise.

* Implements convert(Array, AbstractTriangular) as a short child of convert(Matrix, AbstractTriangular), and reimplements full(AbstractTriangular) as a short child of convert(Array, AbstractTriangular).

* Implements convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) as synonymous children of convert(Matrix, Tridiagonal) and convert(Matrix, SymTridiagonal) respectively, and reimplements full(Tridiagonal) and full(SymTridiagonal) as short children of convert(Array, Tridiagonal) and convert(Array, SymTridiagonal) respectively.
mfasi pushed a commit to mfasi/julia that referenced this issue Sep 5, 2016
@Sacha0
Copy link
Member

Sacha0 commented Oct 24, 2017

Closed by #24250 and the series of preceding pull requests. Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants