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

deprecate orthogonal decomposition keyword "thin" to "full" #24279

Merged
merged 3 commits into from
Oct 28, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Oct 22, 2017

This pull request deprecates the thin keyword argument to orthogonal factorizations in favor of square, addressing #23882. Best!

@Sacha0 Sacha0 added deprecation This change introduces or involves a deprecation linear algebra Linear algebra labels Oct 22, 2017
@Sacha0 Sacha0 added this to the 1.0 milestone Oct 22, 2017
NEWS.md Outdated
@@ -363,6 +363,9 @@ Deprecated or removed
* The `cholfact`/`cholfact!` methods that accepted an `uplo` symbol have been deprecated
in favor of using `Hermitian` (or `Symmetric`) views ([#22187], [#22188]).

* The `thin` keyword argument for orthogonal decomposition methods has
been deprecated in favor of `square` ([#WHATAREBIRDS]).
Copy link
Member

Choose a reason for hiding this comment

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

([#24279])

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks! :)

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 24, 2017

Absent objections or requests for time, I plan to merge these changes tomorrow. Best!

NEWS.md Outdated
@@ -363,6 +363,9 @@ Deprecated or removed
* The `cholfact`/`cholfact!` methods that accepted an `uplo` symbol have been deprecated
in favor of using `Hermitian` (or `Symmetric`) views ([#22187], [#22188]).

* The `thin` keyword argument for orthogonal decomposition methods has
been deprecated in favor of `square` ([#24279]).
Copy link
Member

Choose a reason for hiding this comment

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

which has the opposite meaning: thin=true if and only if square=false.

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 call! Revised accordingly. Thanks!

@Jutho
Copy link
Contributor

Jutho commented Oct 25, 2017

square=false sounds a bit strange given the fact that, if the input matrix is square, the thin output matrices will also be square.

@StefanKarpinski
Copy link
Member

Something about the square terminology here does feel a bit funny.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

Agreed, square lacks something :). Apart from simply retaining "thin" and further documenting that name's oddity in e.g. the LQ context, "reduced" is the remaining possibility that comes to mind. Thoughts? Thanks!

@StefanKarpinski
Copy link
Member

"Reduced" feels a bit better since it's a reduced dimension factorization. Could you humor me and explain in words what this argument means in full generality?

@Jutho
Copy link
Contributor

Jutho commented Oct 26, 2017

For the SVD of a size (m,n) matrix, the most compact form has U of size (m,min(m,n)), S a diagonal of length min(m,n) and Vt of size (min(m,n),n). Matlab has this strange thing where an argument 0 means that U will be reduced, but V is still guaranteed square (and thus S is rectangular if n>m), whereas the argument econ gives both U and V in reduced form.

That as an aside to propose the argument econ with some precedence. However, personally I prefer (and would therefore also propose) compact over econ, reduced or thin.

@StefanKarpinski
Copy link
Member

I thought the whole point of this terminology change was to use consistent terminology across different factorizations, not just SVD. So what's the explanation for the other ones? The point of this exercise is that if you find the common language to naturally describe what this means in a way that applies to all of the usages, then whatever words you use in that explanation are what you should use for the keyword name. If there is no way to succinctly describe what this does across many usage contexts then we should reconsider whether it's actually the same concept or not.

@Jutho
Copy link
Contributor

Jutho commented Oct 26, 2017

It is the same concept, and on second thought I am as happy with reduced as with compact. It seems that in the case of QR, reduced at least has some precedence in Trefethen and Bau as alternative to thin:
https://en.wikipedia.org/wiki/QR_decomposition

@StefanKarpinski
Copy link
Member

It is the same concept

Which I'm asking for evidence of in the form of a common explanation.

@Jutho
Copy link
Contributor

Jutho commented Oct 26, 2017

You can always compute a reduced SVD of an (m,n) matrix A by performing the following steps:

  • compute a reduced QR of A -> Q1 of size (m,min(m,n)), R of size (min(m,n), n)
  • compute a reduced LQ of R -> L of size (min(m,n), min(m,n)), Q2 of size (min(m,n), n)
  • compute a regular SVD of L -> U1 , S and V2' , all of size (min(m,n), min(m,n))
    Set U = Q1*U1
    Set V' = V2'*Q2

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

Each of the common orthogonal(/unitary) decompositions comes in several forms. In the canonical form of each, the orthogonal factors are square and thus proper orthogonal matrices (as opposed to rectangular matrices that contain orthonormal columns). For example, in a canonical SVD decomposition USV of m-by-n matrix A, U and V respectively are m-by-m and n-by-n orthogonal (square) matrices while S is m-by-n. Similarly, in a canonical QR decomposition of m-by-n matrix A, Q is an m-by-m orthogonal (square) matrix while R is m-by-n. This canonical form is often called the "full" form.

When A is rectangular, this canonical form is wasteful in practice: Computing part of the orthogonal factors is unnecessary, burning both decomposition time and storage. Hence in practice another form is common, wherein only the necessary, rectangular part of the "orthogonal" factors is computed and stored. For example, in this form of SVD decomposition USV of m-by-n matrix A, U is an m-by-n (rather than m-by-m) matrix with orthonormal columns, S is n-by-n (rather than m-by-n), and V is again n-by-n. Similarly, in this form of QR decomposition of m-by-n matrix A, Q is an m-by-n (rather than m-by-m) matrix with orthonormal columns, and R is n-by-n (rather than m-by-n). This form has two common names, seemingly propagated by two classic numerical linear algebra textbooks: Golub & van Loan call such decompositions "thin", while Trefethen & Bau call such decompositions "reduced". Those taught from one tend to prefer the one, and the other the other.

There are also distinct "compact" (for efficient decomposition of low-rank matrices) and "truncated" (for approximate low-rank decomposition) forms, separate from those forms at issue here.

Does the above clarify? :)

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Oct 26, 2017

This canonical form is often called the "full" form.

Perhaps this is the answer? Have a full::Bool keyword argument to request the full form of a factorization. Since the full form seems like it's usually not what you want, we could have full default to false. The name full avoids the problem with square=false when the non-full factorization happens to be square, since you're not explicitly asking for something not to be square.

Or in other words: "full" is dead, long live "full"!

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

Cheers, full is much better than square :). Either of full or reduced for the keyword sounds great on this end. Independent of which we choose for the keyword, I propose we use full and reduced consistently in the docstrings. Thoughts? Thanks!

@StefanKarpinski
Copy link
Member

The term "full" seems clearer to me and if the default is not full, then I think it's even better since the full factorization is something that you need to explicitly ask for. The opposite of "full" could be reduced, but that could just be a matter of terminology rather than the name of a keyword.

@StefanKarpinski
Copy link
Member

Let's give other linalg types like @Jutho and @andreasnoack a chance to chime in.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

To be clear, I agree that full seems like the best choice for the keyword at present. Best!

@Jutho
Copy link
Contributor

Jutho commented Oct 26, 2017

Or in other words: "full" is dead. Long live "full"!

Hail to "full"!

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Oct 26, 2017

Ok, seems like we have a consensus: everyone seems to like a full::Bool=false keyword argument for this – defaulting to false since you usually don't want the full factorization. Edit: @Sacha0 is way ahead of me and has already updated this :)

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 26, 2017

Revised with keyword full. (This pull request will leave the docs and comments ripe for some smoothing, but that can happen at a future point.) Thanks all!

@Sacha0 Sacha0 changed the title deprecate orthogonal decomposition keyword "thin" to "square" deprecate orthogonal decomposition keyword "thin" to "full" Oct 26, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 27, 2017

Additional comments? :) Absent objections or requests for time, I plan to merge these changes tomorrow. Best!

@Sacha0 Sacha0 merged commit 2044957 into JuliaLang:master Oct 28, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Oct 28, 2017

Thanks all! :)

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.

4 participants