deprecate lu/eig/schur/svd/lq in favor of *fact counterparts and factorization destructuring by Sacha0 · Pull Request #26997 · JuliaLang/julia

Sacha0 · 2018-05-05T21:00:09Z

(Rewritten with updates:) This pull request concerns JuliaLang/LinearAlgebra.jl#322 and #25187, deprecating lu, eig, schur, svd, and lq to *fact counterparts and factorization destructuring.

Other, slightly different and/or trickier targets for similar deprecation:chol and qr. Best!

ararslan · 2018-05-06T00:46:09Z

stdlib/LinearAlgebra/src/deprecated.jl

+        "`lufact(A[, pivot])` returns an `LU` object. So for a direct replacement, ",
+        "use `(lufact(A[, pivot])...,)`. But going forward, consider using the direct ",
+        "result of `lufact(A[, pivot])` instead, either destructured into its components ",
+        "(`l, u, p = lufact(A[, pivot])`) or as an `LU` object (`lup = lufact(A)`)."))


depwarn takes the name of the function being deprecated as a Symbol in the last argument, so this should have a , :lu) at the end. Same for the method below.

Good catch! I have some rust to grind off :).

…ring.

Sacha0 · 2018-05-13T20:29:29Z

Deprecation warnings fixed, exports removed, and news added. Also added a commit giving eig the same treatment. Working on the same for other decompositions, but the rest are somewhat trickier. Best!

Sacha0 · 2018-05-13T22:26:01Z

Let's break the CI straight flush: Added a commit extending this work to schur and added news for eig. Best!

andreasnoack · 2018-05-14T09:24:23Z

Thanks for doing this. I think we should go with @StefanKarpinski's suggestion though, i.e. use lu, qr, etc instead of the fact versions. That should also resolve JuliaLang/LinearAlgebra.jl#519. If we define, and immediately deprecate, getindex(::<:Factorzation, ::Number) methods then I think this can be done without breaking code.

Sacha0 · 2018-05-14T16:23:48Z

If we define, and immediately deprecate, getindex(::<:Factorzation, ::Number) methods then I think this can be done without breaking code.

Clever! On the one hand, that approach should work for eig, schur, and lu (apart from the lu(x::Number) method, for the reason given in the first commit's NEWS.md entry). On the other hand, avoiding hard breakage may not be possible via that approach with chol, qr, lq, and svd due to their behavior differing appreciably from their *fact counterparts. (For example, svd yields V in U*S*V' whereas svdfact nominally yields Vt in U*S*Vt. qr yields arrays whereas qrfact yields implicit decomposition components, and likewise lq. chol is somewhat different altogether, yielding the decomposition component directly rather than wrapped in a tuple.)

Thoughts? Thanks!

fredrikekre · 2018-05-15T17:48:27Z

stdlib/LinearAlgebra/src/eigen.jl

+  0.0-1.0im   0.0+1.0im
+ -1.0-0.0im  -1.0+0.0im
+
+julia> vals, vecs = F; # destructuring via iterationw


iterationw -> iteration (no big deal, but since this is part of the user facing docs I think it is worth updating)

Great catch! Should be fixed next I push. Thanks! :)

…turing.

…tructuring.

Sacha0 · 2018-05-15T22:24:36Z

svd down as well now. chol, qr, and lq remaining.

Sacha0 · 2018-05-17T02:51:21Z

The one test failure with the most recent (svd) commit should be fixed, which is to say this pull request should be mergeworthy. I hope to extend this work to chol, qr, and lq (any others?) by some time tomorrow, hopefully before the alpha hammer drops :). Best!

…turing.

Sacha0 · 2018-05-17T03:59:22Z

lq live; down to chol and qr. Best!

…ring.

Sacha0 · 2018-05-17T16:56:42Z

Small note: chol has a method for UniformScaling. UniformScalings cannot be wrapped in Choleskys. So either we simply nix this method and recommend S -> UniformScaling(sqrt(S.λ)) or replace it with a new cholfact method that returns a UniformScaling (a drop-in replacement). The latter seems preferable, so implementing the latter for now. Best!

Edit: Or perhaps it's best to keep this chol method, as in 1.0 we would deprecate the new, possibly iffy cholfact method back to chol. No point in the shuffle.

fredrikekre · 2018-05-17T17:09:31Z

chol has a method for UniformScaling.

IIRC this was introduced in 0.7, so perhaps just nix it for simplicity?

Sacha0 · 2018-05-17T17:21:46Z

IIRC this was introduced in 0.7, so perhaps just nix it for simplicity?

(Your post coincided with my edit it seems 😄.) Absolutely, just removing this method would be much better than deprecating it to cholfact. Then we are left with the question: Should we remove this method, or simply leave it in anticipation of later reintroduction? Best!

(Ref. pull request that introduced this method: #22633.)

Sacha0 · 2018-05-17T18:49:09Z

(AV x86_64 timed out, and Circle x86_64 hit unrelated LibGit2 errors.)

Sacha0 · 2018-05-17T19:39:04Z

Triage favors rewriting this pull request embracing the joy of breakage: Give the *fact semantics to the non-*fact names immediately in 0.7, and scour from the earth all methods that stand in the way. The red flag has been raised, and no quarter shall be granted.

Sacha0 · 2018-05-21T01:46:06Z

Superseded by #27159.

mschauer · 2018-05-24T19:46:10Z

You could have pinged me because of d: #22633.

Sacha0 added linear algebra Linear algebra needs news A NEWS entry is required for this change deprecation This change introduces or involves a deprecation labels May 5, 2018

ararslan reviewed May 6, 2018

View reviewed changes

Sacha0 force-pushed the fact branch from 616ff0d to 9d83720 Compare May 13, 2018 18:08

Sacha0 removed the needs news A NEWS entry is required for this change label May 13, 2018

Deprecate lu(...) in favor of lufact(...) and factorization destructu…

76e77d8

…ring.

Sacha0 force-pushed the fact branch 2 times, most recently from f4ebabe to 78344d1 Compare May 13, 2018 20:15

Sacha0 changed the title ~~deprecate lu(...) in favor of lufact(...) and factorization destructuring~~ deprecate [lu](...) in favor of lufact(...) and factorization destructuring May 13, 2018

Sacha0 changed the title ~~deprecate [lu](...) in favor of lufact(...) and factorization destructuring~~ deprecate [lu|eig](...) in favor of [lu|eig]fact(...) and factorization destructuring May 13, 2018

Sacha0 force-pushed the fact branch from 78344d1 to 1624676 Compare May 13, 2018 22:26

Sacha0 force-pushed the fact branch from 1624676 to b2c3772 Compare May 14, 2018 02:57

Sacha0 mentioned this pull request May 15, 2018

decomposition and factorization terminology JuliaLang/LinearAlgebra.jl#519

Closed

fredrikekre reviewed May 15, 2018

View reviewed changes

Sacha0 added 2 commits May 15, 2018 10:56

Deprecate eig(...) in favor of eigfact(...) and factorization destruc…

37782d5

…turing.

Deprecate schur(...) in favor of schurfact(...) and factorization des…

3c94a27

…tructuring.

Sacha0 force-pushed the fact branch from b2c3772 to 8fedd69 Compare May 15, 2018 22:22

Sacha0 changed the title ~~deprecate [lu|eig|schur](...) in favor of [lu|eig|schur]fact(...) and factorization destructuring~~ deprecate lu/eig/schur/svd in favor of *fact counterparts and factorization destructuring May 15, 2018

Sacha0 force-pushed the fact branch from 8fedd69 to 087be3b Compare May 17, 2018 02:47

Sacha0 mentioned this pull request May 17, 2018

State of inner products in Base JuliaLang/LinearAlgebra.jl#496

Closed

Deprecate svd(...) in favor of svdfact(...) and factorization destruc…

0d55fe8

…turing.

Sacha0 force-pushed the fact branch from 087be3b to fac3afe Compare May 17, 2018 03:58

Sacha0 changed the title ~~deprecate lu/eig/schur/svd in favor of *fact counterparts and factorization destructuring~~ deprecate lu/eig/schur/svd/lq in favor of *fact counterparts and factorization destructuring May 17, 2018

JeffBezanson added the triage This should be discussed on a triage call label May 17, 2018

Deprecate lq(...) in favor of lqfact(...) and factorization destructu…

4aed581

…ring.

Sacha0 force-pushed the fact branch from fac3afe to 4aed581 Compare May 17, 2018 16:46

JeffBezanson added this to the 1.0 milestone May 17, 2018

JeffBezanson removed the triage This should be discussed on a triage call label May 17, 2018

This was referenced May 18, 2018

RFC: Avoid over-pessimization in apply_type_tfunc #27150

Merged

break all the decompositions #27159

Closed

Sacha0 closed this May 21, 2018

Sacha0 deleted the fact branch May 21, 2018 01:46

Sacha0 removed this from the 1.0 milestone May 21, 2018

Sacha0 mentioned this pull request May 23, 2018

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

Merged

7 tasks

Uh oh!

Conversation

Sacha0 commented May 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ararslan May 6, 2018

Choose a reason for hiding this comment

Uh oh!

Sacha0 May 6, 2018

Choose a reason for hiding this comment

Uh oh!

Sacha0 commented May 13, 2018

Uh oh!

Sacha0 commented May 13, 2018

Uh oh!

andreasnoack commented May 14, 2018

Uh oh!

Sacha0 commented May 14, 2018

Uh oh!

fredrikekre May 15, 2018

Choose a reason for hiding this comment

Uh oh!

Sacha0 May 15, 2018

Choose a reason for hiding this comment

Uh oh!

Sacha0 commented May 15, 2018

Uh oh!

Sacha0 commented May 17, 2018

Uh oh!

Sacha0 commented May 17, 2018

Uh oh!

Sacha0 commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredrikekre commented May 17, 2018

Uh oh!

Sacha0 commented May 17, 2018

Uh oh!

Sacha0 commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sacha0 commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sacha0 commented May 21, 2018

Uh oh!

mschauer commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sacha0 commented May 5, 2018 •

edited

Loading

Sacha0 commented May 17, 2018 •

edited

Loading

Sacha0 commented May 17, 2018 •

edited

Loading

Sacha0 commented May 17, 2018 •

edited

Loading