break all the decompositions

[Sacha0]

This pull request implements the new approach to #26997. That is, this pull request deprecates lufact[!], eigfact[!], schurfact[!], lqrfact[!], qrfact[!], bkfact[!], cholfact[!], ldltfact[!], hessfact[!], and svdfact[!] to lu[!], eig[!], schur[!], lq[!], qr[!], bk[!], chol[!], ldlt[!], hess[!], and svd[!] respectively, clobbering existing methods of the latter functions. To facilitate this deprecation, this pull request adds destructuring via iteration to decomposition objects, and getindex(S::($DecompositionType), i::Integer) methods that yield the decomposition components produced by iteration.

Decision points: 1) Should the getindex(S::$(Decomposition), i::Integer) methods be left in place indefinitely, or immediately deprecated? Deprecated, and done. 2) Should we deprecate e.g. lufact! to lu! simultaneously? Yes, and done.

Best!

[Keno]

This throws the type, you want to throw an appropriate instance.

[Sacha0]

Good catch! Fixed on next push. Thanks! :)

[ararslan]

Since it's a simple renaming rather than an API change, you can do the same thing as was done with trace above and just call Base.@deprecate_binding lufact lu

[Sacha0]

I hesitate to broaden the deprecated signatures as *fact methods quite possibly exist outside stdlib. Thoughts?

[fredrikekre]

Deprecations from @deprecate_binding are impossible to find since they don't show a location.

[Sacha0]

Now covering eig/eigfact.

[fredrikekre]

1. Should the getindex(S::$(Decomposition), i::Integer) methods be left in place indefinitely, or immediately deprecated?

Just to clarify, the reason we have those methods is to not break things like

x = lufact(A)
u = x[2]

right? Then I say we deprecate them directly. IMO it is much cleaner to do

l, u, p = lufact(A)

or

x = lufact(A)
u = x.U

anyways, and we don't need to provide yet another way to access the parts...

2. Should we deprecate e.g. lufact! to lu! simultaneously?

Yes!

[Sacha0]

Just to clarify, the reason we have those methods is to not break things like

x = lufact(A)
u = x[2]

Almost! :) Rather to avoid breaking

x = lu(A)
u = x[2]

as lu presently returns a tuple, whereas lufact(A)[2] should presently throw.

Then I say we deprecate them directly. IMO it is much cleaner to do [...] anyways, and we don't need to provide yet another way to access the parts [...]

My sentiments mirror yours :). Absent countervailing opinions, I will add deprecations for those methods once everything else is in order. Much thanks!

[Sacha0]

Now covering schur/schurfact.

[StefanKarpinski]

Am I correct in assuming that the *fact! deprecations are still to come here? Presumably lufact!(X) will be deprecated to lu!(X) or something like that?

[Sacha0]

Am I correct in assuming that the *fact! deprecations are still to come here? Presumably lufact!(X) will be deprecated to lu!(X) or something like that?

Correct! :) I plan to work through the remaining *fact operations first, then clear the *fact! equivalents. Best!

[Sacha0]

Now covering lq/lqfact.

[Sacha0]

Now covers all mutating variants, including svdfact!. So only svd/svdfact remain.

[Sacha0]

A bit more detail regarding svd/svdfact: For the other decompositions, with the changes in this pull request code should either continue to work correctly or fail loudly. For svd/svdfact, if we replace the existing svd methods with svdfact+SVD-iteration, code will often continue to work but silently yield incorrect results (due to existing svd methods returning (U, S, V) while the natural components to produce from iterating an SVD are (U, S, Vt).

Proceeding nonetheless per triage, but just to make certain this pitfall is well known :).

[StefanKarpinski]

Can't we just have the factorization object retuned by svd iterate U, S and V as is traditional? We can have both svd(X).V and svd(X).Vt accessors. Since the transpose operation is done lazily I'm not sure what the drawback would be.

[Sacha0]

Can't we just have the factorization object retuned by svd iterate U, S and V as is traditional? We can have both svd(X).V and svd(X).Vt accessors. Since the transpose operation is done lazily I'm not sure what the drawback would be.

We could, yes, and I have been leaning towards that approach as well. Happily we already have V and Vt accessors, and the V accessor is lazy now as you would hope. If this approach seems reasonable to you as well, I'll happily roll with it :).

[StefanKarpinski]

It seems fine to me. Sometimes you want V, sometimes you want V'; I don't think either one is uniformly better to yield via iteration.

[Sacha0]

It seems fine to me. Sometimes you want V, sometimes you want V'; I don't think either one is uniformly better to yield via iteration.

I'm sold then :). Laziness can be such a wonderful thing!

[Sacha0]

This pull request now covers all decompositions (mutating variants included) and deprecates the transitional getindex methods in the final commit. Modulo the last two commits (which I am waiting for CI on, and expect a warning or two from the last commit that will need fixing), all intermediate commits pass tests (and are independently meaningful, so I would not squash by default). So from my perspective this pull request should be in reasonable shape once CI is green. Best!

[fredrikekre]

Proposal: Spell out bk, hess and chol to bunchkaufman, hessenberg and cholesky (also less breaking, but that is secondary). Personally I think that

x = cholesky(A)\b

looks pretty nice.

Edit: Additionally @Sacha0 suggests we should consider eig -> eigen.

[StefanKarpinski]

Being a little less cryptic with some of these factorization names would not hurt.

[Sacha0]

Agreed, bk and hess are unfortunate; bunchkaufman and hessenberg would be better. I prefer cholesky to chol as well, though I find it less unfortunate than the former two. ldlt seems not great as well, though I'm not certain what would be better. Even expanding eig to eigen strikes me as reasonable. I would be happy to put together a followup if support is widespread. Best!

[ViralBShah]

I would definitely rename bk. It is terrible to take up a 2 letter name. I would be onboard with renaming hess too, and then for completeness even chol and eig. Those last two seem like they are more entrenched names, but the full names would be nice for consistency.

[Sacha0]

Cheers for consensus on the renames! :)

I would prefer to do the renames in downstream pull requests. So absent objections or requests for time, I plan to merge this pull request shortly after CI for the last commit clears (perhaps this evening or tomorrow morning PST). Best!

[Sacha0]

@simonbyrne (much thanks!) observed that these changes will break code like chol(A) \ x: presently chol(A) \ x == U \ x for A = U'U, rather than cholfact(A) \ x == U \ ( U' \ x). Back to the grind stone... :)

[ViralBShah]

I guess this is where using cholesky will help easily deprecate the old behaviour.

[Sacha0]

Superseded by #27212, which will incorporate the renames to reduce breakage. Much thanks for the fantastic input all, and particularly @fredrikekre and @simonbyrne for pivotal pieces! :)