{"action":"created","issue":{"url":"https://api.github.com/repos/JuliaLang/julia/issues/17066","repository_url":"https://api.github.com/repos/JuliaLang/julia","labels_url":"https://api.github.com/repos/JuliaLang/julia/issues/17066/labels{/name}","comments_url":"https://api.github.com/repos/JuliaLang/julia/issues/17066/comments","events_url":"https://api.github.com/repos/JuliaLang/julia/issues/17066/events","html_url":"https://github.com/JuliaLang/julia/pull/17066","id":161785133,"number":17066,"title":"Towards #12251, reimplement all full(X) method defs. as convert(Array, X)","user":{"login":"Sacha0","id":5799177,"avatar_url":"https://avatars.githubusercontent.com/u/5799177?v=3","gravatar_id":"","url":"https://api.github.com/users/Sacha0","html_url":"https://github.com/Sacha0","followers_url":"https://api.github.com/users/Sacha0/followers","following_url":"https://api.github.com/users/Sacha0/following{/other_user}","gists_url":"https://api.github.com/users/Sacha0/gists{/gist_id}","starred_url":"https://api.github.com/users/Sacha0/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Sacha0/subscriptions","organizations_url":"https://api.github.com/users/Sacha0/orgs","repos_url":"https://api.github.com/users/Sacha0/repos","events_url":"https://api.github.com/users/Sacha0/events{/privacy}","received_events_url":"https://api.github.com/users/Sacha0/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2016-06-22T21:02:01Z","updated_at":"2016-06-23T17:24:39Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/JuliaLang/julia/pulls/17066","html_url":"https://github.com/JuliaLang/julia/pull/17066","diff_url":"https://github.com/JuliaLang/julia/pull/17066.diff","patch_url":"https://github.com/JuliaLang/julia/pull/17066.patch"},"body":"First step towards #12251 (edit: explanation and plan now described in https://github.com/JuliaLang/julia/pull/17066#issuecomment-228113687). This pull request reduces every `full(X)` method definition to `convert(Array, X)` (apart from the `AbstractArray` no-op fallback and the `approx_full` methods in `test.jl`):\r\n```\r\n~/pkg/julia$ grep -rn -e 'function full[{(]' -e 'full\\({.*}\\)\\?(.*).* =' base/\r\nbase//abstractarray.jl:663:full(x::AbstractArray) = x\r\nbase//linalg/bidiag.jl:107:full(A::Bidiagonal) = convert(Array, A)\r\nbase//linalg/cholesky.jl:329:full(C::Cholesky) = convert(Array, C)\r\nbase//linalg/cholesky.jl:333:full(F::CholeskyPivoted) = convert(Array, F)\r\nbase//linalg/diagonal.jl:28:full(D::Diagonal) = convert(Array, D)\r\nbase//linalg/eigen.jl:190:full(F::Eigen) = convert(Array, F)\r\nbase//linalg/hessenberg.jl:58:full(A::HessenbergQ) = convert(Array, A)\r\nbase//linalg/hessenberg.jl:61:full(F::Hessenberg) = convert(Array, F)\r\nbase//linalg/ldlt.jl:91:full(F::LDLt) = convert(Array, F)\r\nbase//linalg/lq.jl:93:full(A::LQ) = convert(Array, A)\r\nbase//linalg/lq.jl:109:full(A::LQPackedQ; thin::Bool = true) = convert(Array, A, thin = thin)\r\nbase//linalg/lu.jl:438:full(F::LU) = convert(Array, F)\r\nbase//linalg/lu.jl:478:full{T}(F::Base.LinAlg.LU{T,Tridiagonal{T}}) = convert(Array, F)\r\nbase//linalg/qr.jl:179:full(F::Union{QR,QRCompactWY}) = convert(Array, F)\r\nbase//linalg/qr.jl:184:full(F::QRPivoted) = convert(Array, F)\r\nbase//linalg/qr.jl:252:full(A::Union{QRPackedQ,QRCompactWYQ}; thin::Bool = true) = convert(Array, A, thin = thin)\r\nbase//linalg/schur.jl:212:full(F::Schur) = convert(Array, F)\r\nbase//linalg/svd.jl:242:full(F::SVD) = convert(Array, F)\r\nbase//linalg/symmetric.jl:87:full(A::Union{Symmetric,Hermitian}) = convert(Array, A)\r\nbase//linalg/triangular.jl:52:full(A::AbstractTriangular) = convert(Array, A)\r\nbase//linalg/tridiag.jl:58:full(M::SymTridiagonal) = convert(Array, M)\r\nbase//linalg/tridiag.jl:382:full(M::Tridiagonal) = convert(Array, M)\r\nbase//sparse/sparsematrix.jl:293:full(S::SparseMatrixCSC) = convert(Array, S)\r\nbase//sparse/sparsevector.jl:679:full(x::AbstractSparseVector) = convert(Array, x)\r\nbase//test.jl:821:approx_full(x::AbstractArray) = x\r\nbase//test.jl:822:approx_full(x::Number) = x\r\nbase//test.jl:823:approx_full(x) = full(x)\r\n```\r\nThis change should enable drop-in replacement of `full(X)` calls with `convert(Array, X)` throughout `base/`, the next step towards #12251. Some notes:\r\n\r\nEach `full(X)` method definition is now part of a block like\r\n```julia\r\nconvert(::Type{Matrix}, A::SparseMatrixCSC) = ...\r\nconvert(::Type{Array}, A::SparseMatrixCSC) = convert(Matrix, A)\r\nfull(A::SparseMatrixCSC) = convert(Array, A)\r\n```\r\nor\r\n```julia\r\nconvert(::Type{Vector}, x::AbstractSparseVector) = ...\r\nconvert(::Type{Array}, x::AbstractSparseVector) = convert(Vector, x)\r\nfull(x::AbstractSparseVector) = convert(Array, x)\r\n```\r\nsuch that generic methods may call `convert(Array, foo)` for arbitrary presently-`full`-able `foo`, and hence don't need to know whether `foo` naturally converts to a `Matrix`, `Vector`, etc. In some cases parametric `convert{T}(::Type{Matrix{T}}, X)` methods already existed; this pull request preserves those methods and implements the `convert(::Type{Matrix}, X)` and `convert(::Type{Array}, X)` methods as children thereof. But it does not introduce such parametric definitions where they did not previously exist. Thoughts?\r\n\r\nThe `LDLt` type previously returned a `SymTridiagonal` rather than `Matrix` from `full`, exhibiting the semantics discussed in #12153 rather than the more typical `full` semantics. Same for certain `LU` types. In those cases, this pull request starts on #12153 by, for example in the case of `LDLt`, replacing the former `full` functionality with a block like\r\n```julia\r\nconvert(::Type{SymTridiagonal}, F::LDLt) = ....\r\nconvert(::Type{AbstractMatrix}, F::LDLt) = convert(SymTridiagonal, F)\r\nconvert(::Type{AbstractArray}, F::LDLt) = convert(AbstractMatrix, F)\r\n```\r\nand implementing `convert(::Type{Matrix}, F::LDLt)` as a short child of `convert(::Type{AbstractArray}, F::LDLt)`. Thoughts?\r\n\r\nFor `Symmetric`/`Hermitian` types, the former `full` semantics also differed from the norm:\r\n```julia\r\nfull(A::Symmetric) = copytri!(copy(A.data), A.uplo)\r\n```\r\nwhich does not necessarily return an `Array`. This pull request effectively changes the behavior to\r\n```julia\r\nfull(A::Symmetric) = copytri!(convert(Array, copy(A.data)), A.uplo)\r\n```\r\nThis change leaves a void where the (potentially useful) former behavior lived. Perhaps this behavior needs a separate name, and perhaps unification with the conceptually similar `full!` methods for `XTriangular` types, all of which return a copy of the underlying data modified such that it matches (in the `AbstractArray` comparison sense) the `Symmetric`/`Hermitian`/`XTriangular`-wrapped object. Thoughts?\r\n\r\nBest!"},"comment":{"url":"https://api.github.com/repos/JuliaLang/julia/issues/comments/228121172","html_url":"https://github.com/JuliaLang/julia/pull/17066#issuecomment-228121172","issue_url":"https://api.github.com/repos/JuliaLang/julia/issues/17066","id":228121172,"user":{"login":"nalimilan","id":1120448,"avatar_url":"https://avatars.githubusercontent.com/u/1120448?v=3","gravatar_id":"","url":"https://api.github.com/users/nalimilan","html_url":"https://github.com/nalimilan","followers_url":"https://api.github.com/users/nalimilan/followers","following_url":"https://api.github.com/users/nalimilan/following{/other_user}","gists_url":"https://api.github.com/users/nalimilan/gists{/gist_id}","starred_url":"https://api.github.com/users/nalimilan/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nalimilan/subscriptions","organizations_url":"https://api.github.com/users/nalimilan/orgs","repos_url":"https://api.github.com/users/nalimilan/repos","events_url":"https://api.github.com/users/nalimilan/events{/privacy}","received_events_url":"https://api.github.com/users/nalimilan/received_events","type":"User","site_admin":false},"created_at":"2016-06-23T17:24:39Z","updated_at":"2016-06-23T17:24:39Z","body":"> If migrating the full tests to the underlying convert methods at this stage would be better than doing so in step (2) as described above, I would be happy to do so. Thoughts? Thanks, and best!\r\n\r\nAdding untested features isn't generally a good idea. I guess you could copy the existing tests for `full` to make them exercize `convert` immediately, and then remove the old ones testing `full` as part of (2)."}}
{"actor":{"display_login":"nalimilan"}}