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.

Sign up for GitHub

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

Jump to bottom

deprecate lu/eig/schur/svd/lq in favor of *fact counterparts and factorization destructuring #26997

Closed
wants to merge 5 commits into from
Closed

deprecate lu/eig/schur/svd/lq in favor of *fact counterparts and factorization destructuring #26997

wants to merge 5 commits into from

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented May 5, 2018 •
edited
Loading

(Rewritten with updates:) This pull request concerns #15573 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!

@Sacha0 Sacha0 added domain:linear algebra Linear algebra needs news A NEWS entry is required for this change kind:deprecation This change introduces or involves a deprecation labels May 5, 2018
ararslan
ararslan reviewed May 6, 2018
View reviewed changes
stdlib/LinearAlgebra/src/deprecated.jl Outdated
"`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)`)."))
Copy link
Member

Choose a reason for hiding this comment

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

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.

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! I have some rust to grind off :).

@Sacha0 Sacha0 removed the needs news A NEWS entry is required for this change label May 13, 2018
@Sacha0 Sacha0 force-pushed the fact branch 2 times, most recently from f4ebabe to 78344d1 Compare May 13, 2018 20:15
@Sacha0
Copy link
Member Author

Sacha0 commented May 13, 2018

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 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 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
Copy link
Member Author

Sacha0 commented May 13, 2018

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

@Sacha0 Sacha0 changed the title deprecate [lu|eig](...) in favor of [lu|eig]fact(...) and factorization destructuring deprecate [lu|eig|schur](...) in favor of [lu|eig|schur]fact(...) and factorization destructuring May 13, 2018
@andreasnoack
Copy link
Member

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 #26995. If we define, and immediately deprecate, getindex(::<:Factorzation, ::Number) methods then I think this can be done without breaking code.

@Sacha0
Copy link
Member Author

Sacha0 commented May 14, 2018

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!

@Sacha0 Sacha0 mentioned this pull request May 15, 2018
Closed
fredrikekre
fredrikekre reviewed May 15, 2018
View reviewed changes
stdlib/LinearAlgebra/src/eigen.jl Outdated
0.0-1.0im 0.0+1.0im
-1.0-0.0im -1.0+0.0im

julia> vals, vecs = F; # destructuring via iterationw
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Sacha0 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
Copy link
Member Author

Sacha0 commented May 15, 2018

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

@Sacha0
Copy link
Member Author

Sacha0 commented May 17, 2018

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!

@Sacha0 Sacha0 mentioned this pull request May 17, 2018
Closed
@Sacha0 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
@Sacha0
Copy link
Member Author

Sacha0 commented May 17, 2018

lq live; down to chol and qr. Best!

@JeffBezanson JeffBezanson added the status:triage This should be discussed on a triage call label May 17, 2018
@Sacha0
Copy link
Member Author

Sacha0 commented May 17, 2018 •
edited
Loading

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
Copy link
Member

chol has a method for UniformScaling.

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

@Sacha0
Copy link
Member Author

Sacha0 commented May 17, 2018

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
Copy link
Member Author

Sacha0 commented May 17, 2018 •
edited
Loading

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

@JeffBezanson JeffBezanson added this to the 1.0 milestone May 17, 2018
@JeffBezanson JeffBezanson removed the status:triage This should be discussed on a triage call label May 17, 2018
@Sacha0
Copy link
Member Author

Sacha0 commented May 17, 2018 •
edited
Loading

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.

This was referenced May 18, 2018
Merged
Closed
@Sacha0
Copy link
Member Author

Sacha0 commented May 21, 2018

Superseded by #27159.

@Sacha0 Sacha0 closed this May 21, 2018
@Sacha0 Sacha0 deleted the fact branch May 21, 2018 01:46
@Sacha0 Sacha0 removed this from the 1.0 milestone May 21, 2018
@Sacha0 Sacha0 mentioned this pull request May 23, 2018
Merged
7 tasks
@mschauer
Copy link
Contributor

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

@garrison garrison mentioned this pull request Aug 3, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ararslan ararslan ararslan left review comments

@fredrikekre fredrikekre fredrikekre left review comments

Assignees
No one assigned
Labels
domain:linear algebra Linear algebra kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Sacha0 @andreasnoack @fredrikekre @mschauer @ararslan @JeffBezanson