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

getindex(::Factorization) not type stable #241

Closed
jiahao opened this issue Aug 22, 2015 · 1 comment
Closed

getindex(::Factorization) not type stable #241

jiahao opened this issue Aug 22, 2015 · 1 comment
Labels
breaking This change will break code needs decision A decision on this change is needed

Comments

@jiahao
Copy link
Member

jiahao commented Aug 22, 2015

The current idiom for accessing components of a matrix Factorization object is by indexing. For example, eigfact(M)[:values] returns the eigenvalues of M after computing its spectral factorization, which is stored in an Eigen type. However, eigfact(M)[:values] returns a vector while eigfact(M)[:vectors] returns a matrix. The result is that getindex(::Eigen) is not type stable and static analysis cannot infer a concrete return type.

Consequently, linear algebra code building upon computed matrix factors must either make special effort to avoid the type instability, or break the abstraction of the Factorization object by directly accessing its fields, or else risk degraded performance if the lack of a concrete return type pollutes type inference downstream.

See

and also this simple code illustrating the problem, annotated with the inferred types given by code_warntype:

function svdvals_in_ascending_order(A::Matrix{Float64})
    S = svdfact(A) #SVD{Float64,Float64,Matrix{Float64}}
    Σ = S[:S] #Union{Array{Float64,1},Array{Float64,2}}
    sort!(Σ, rev=false) #Any; method with kwarg is hard for Julia to analyze
end

It seems like we should think more about whether the getindex idiom makes sense when the Factorization object consists of heterogeneous fields. I vaguely recall this issue being raised before, but I couldn't find it on GitHub.

@jiahao jiahao added breaking This change will break code linear algebra needs decision A decision on this change is needed labels Aug 22, 2015
@andreasnoack
Copy link
Member

Dup of #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

2 participants