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

once more unto the decomposition breach, dear friends! #27212

Merged
merged 23 commits into from
May 24, 2018

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented May 23, 2018

...or close up the wall with our obscure initialisms!

This pull request is take three on the recent decompositions rework; take two was #27159, and take one was #26997.

Specifically, this pull request deprecates lufact[!], schurfact[!], lqfact[!], qrfact[!], ldltfact[!], and svdfact[!] to lu[!], schur[!], lq[!], qr[!], ldlt[!], and svd[!] respectively, clobbering existing methods of the latter functions.

Additionally, this pull request (will eventually) deprecate eig/eigfact[!] to eigen[!], chol[!]/cholfact[!] to cholesky[!], bkfact[!] to bunchkaufman[!], and hessfact[!] to hessenberg[!].

To facilitate this deprecation, this pull request adds destructuring via iteration to some of the decomposition objects, and (immediately deprecated) getindex(S::($DecompositionType), i::Integer) methods that yield the decomposition components produced by iteration.

Pending items:

  • Deprecate bkfact[!] to bunchkaufman[!]
  • Deprecate hessfact[!] to hessenberg[!]
  • Deprecate eigfact[!] to eigen[!]
  • Deprecate cholfact[!] to cholesky[!]
  • Deprecate eig to eigen
  • Deprecate chol[!] to cholesky[!]
  • Deprecate transitional getindex methods

Best!

@Sacha0 Sacha0 added linear algebra Linear algebra deprecation This change introduces or involves a deprecation labels May 23, 2018
@Sacha0 Sacha0 added this to the 1.0 milestone May 23, 2018
@Sacha0 Sacha0 force-pushed the breakless branch 2 times, most recently from 792170c to 6927882 Compare May 23, 2018 04:13
@Sacha0
Copy link
Member Author

Sacha0 commented May 23, 2018

To my surprise and delight, I was able to finish everything this evening except for the gentler chol[!] -> cholesky[!] deprecation. I hope to eke out time for any issues CI catches tomorrow morning, and will try for the preceding last deprecation then as well. If I don't manage that last commit prior to other obligations, I will try to eke out additional time between those obligations, but otherwise pass the torch to @mbauman till Thursday :).

@fredrikekre
Copy link
Member

Feel free to cherry pick from https://github.com/JuliaLang/julia/commits/breakmore

@JeffBezanson JeffBezanson removed this from the 1.0 milestone May 23, 2018
@JeffBezanson JeffBezanson added this to the 0.7 milestone May 23, 2018
@@ -1,4 +1,4 @@
# This file is a part of Julia. License is MIT: https://julialang.org/license
# This file is a part of Julia. License is MIT: https://julialang.org/license
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Should be fixed :).

@@ -7,6 +7,7 @@ using Base: @deprecate, depwarn
@deprecate cond(F::LinearAlgebra.LU, p::Integer) cond(convert(AbstractArray, F), p)

# PR #22188
export cholfact, cholfact!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The @deprecate macro does the exporting for you by default

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed so :). I had forgotten at first, and once I remembered halfway through I decided to proceed consistently. Please feel welcome to remove the explicit exports!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a major concern by any means — just thought I'd mention it as I was reviewing.

# deprecate eig(...) in favor of eigfact and factorization destructuring
export eig
function eig(A::Union{Number, StridedMatrix}; permute::Bool=true, scale::Bool=true)
depwarn(string("`eig(A[, permute, scale])` has been deprecated in favor of ",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this eig(A[, permute, scale]) notation a little confusing — maybe just literally `eig(A; ...)`?

end

# deprecate transitional decomposition getindex methods out of the blocks
@inline function Base.getindex(S::LU, i::Integer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and below) cannot be inlined for the deprecation to work. depwarn needs to have a stack frame to do its warn-once tracking.

Hessenberg(A::StridedMatrix) = Hessenberg(LAPACK.gehrd!(A)...)

# iteration for destructuring into components
Base.iterate(S::Hessenberg) = (S.Q, Val(:H))
Base.iterate(S::Hessenberg, ::Val{:H}) = (S.H, Val(:done))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the others) is quite clever. I'm still catching up with all the things the new optimizer can do, but this seems like it'll be out of its reach and add quite a bit of dynamic dispatch. Maybe just:

+Base.iterate(S::Hessenberg) = (S.Q, false)
+Base.iterate(S::Hessenberg, isdone) = isdone ? nothing : (S.H, true)

Or for the bigger factorizations, this might have a better chance of piggy-backing on the inference magic for tuples (it doesn't quite get it right now, though):

@inline function Base.iterate(x::S, state=nothing)
    tup = (x.a, x.b, x.c)
    state===nothing ? iterate(tup) : iterate(tup, state)
end

@Sacha0
Copy link
Member Author

Sacha0 commented May 23, 2018

(Much thanks @fredrikekre and @mbauman! :D)

@Sacha0
Copy link
Member Author

Sacha0 commented May 23, 2018

Incorporated Matt's getindex @inline removals into the relevant earlier commit, fixed the one CI failure in the SparseArrays suite, and removed the errant whitespace Matt found, so all commits up to "Expand pull request numbers for some news items" should be passing CI now.

Had a brief look at one of Fred's commits: I am not certain that sqrt is generally substitutable for chol in choltype, hence in the relevant commit of #27159 I kept chol[!] around internally as legacychol[!] for the moment. Might be overly conservative though :).

Out of time now, hope to find bits of time again in a couple of hours. Thanks all and best!

@simonbyrne
Copy link
Contributor

It should be possible to deprecate chol(X) to cholesky(X).U

@fredrikekre
Copy link
Member

I am not certain that sqrt is generally substitutable for chol in choltype

Maybe not, but we should only have to keep the legacy_chol method for Number, since choltype calls promote_type(..., Float32) which only makes sense for Numbers (right?). I think it boils down to redefining choltype as:

function choltype(A)
    x = one(eltype(A))
    promote_type(typeof(x), typeof(sqrt(abs(real(x)))), Float32)
end

and then we don't need chol or legacychol.

It should be possible to deprecate chol(X) to cholesky(X).U

62e0b7e

@StefanKarpinski
Copy link
Member

Failures are related to Pkg3 being renamed and/or timeouts.

@Jutho
Copy link
Contributor

Jutho commented Aug 21, 2018

I very much like the new factorization interface and all the work that has been done here. Congrats to everbody involved.

I do have one question. I tried to look for a discussion about the name change eig to eigen. Why was this decided or deemed necessary? svd is also a three letter word so that can be it. I like the name changes from chol to cholesky and the full name hessenberg, but eig seems so standard. It's also used by Matlab and Numpy. In fact, if it were not for eigen, the names would be identical to Numpy/Scipy: numpy.linalg.eig, numpy.linalg.svd, numpy.linalg.cholesky, scipy.linalg.hessenberg, scipy.linalg.schur.

@nalimilan
Copy link
Member

@KristofferC
Copy link
Member

There are still no docs for iterate on factorization objects?

Keno pushed a commit that referenced this pull request Jun 5, 2024
once more unto the decomposition breach, dear friends!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants