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

matrix/mat64,stat: reverse matrix extraction #28

Merged
merged 4 commits into from
May 31, 2017
Merged

Conversation

kortschak
Copy link
Member

Port of gonum/matrix#436.

@btracey Please take a look.

@kortschak kortschak requested a review from btracey May 26, 2017 22:39
Copy link
Member

@btracey btracey left a comment

Choose a reason for hiding this comment

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

LGTM except I would remove the To from the signatures.

@@ -58,10 +58,15 @@ func (lq *LQ) Factorize(a Matrix) {
// TODO(btracey): Add in the "Reduced" forms for extracting the m×m orthogonal
// and upper triangular matrices.

// LFromLQ extracts the m×n lower trapezoidal matrix from a LQ decomposition.
func (m *Dense) LFromLQ(lq *LQ) {
// LTo extracts the m×n lower trapezoidal matrix from a LQ decomposition.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the To on the function calls? It's pretty clear from the function signature alone that it produces the matrix L, and very clear with the documentation. Seeing x := lq.L(nil) or lq.L(x) both feel well in line with our other signatures.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reminding me. Yes, we do - see comments gonum/matrix#436 (comment).

I will go through the other instances that are like this and change them to match. For example gonum/matrix#433 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for reminding me. I still buy the argument.

@kortschak
Copy link
Member Author

Added GSVD and HOGSVD equivalents. PTAL

(also some missed doc errors in previous commits)

// UFromGSVD extracts the matrix U from the singular value decomposition, storing
// the result in-place into the receiver. U is size r×r.
func (m *Dense) UFromGSVD(gsvd *GSVD) {
// UTo extracts the matrix U from the singular value decomposition, storing
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says U is rxr, but this makes it rxc.

Copy link
Member

Choose a reason for hiding this comment

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

The HOGSVD types have a "VTo will panic if the receiver does not contain a successful factorization." Should the GSVD types as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The documentation says U is rxr, but this makes it rxc.

U is (A_r,A_c), so U_r = U_c = r.

The HOGSVD types have a "VTo will panic if the receiver does not contain a successful factorization." Should the GSVD types as well?

Will do.

// VFromGSVD extracts the matrix V from the singular value decomposition, storing
// the result in-place into the receiver. V is size p×p.
func (m *Dense) VFromGSVD(gsvd *GSVD) {
// VTo extracts the matrix V from the singular value decomposition, storing
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says V is pxp, but this makes it rxc.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

// QFromGSVD extracts the matrix Q from the singular value decomposition, storing
// the result in-place into the receiver. Q is size c×c.
func (m *Dense) QFromGSVD(gsvd *GSVD) {
// QTo extracts the matrix Q from the singular value decomposition, storing
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says Q is cxc, but this makes it rxc.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@kortschak kortschak merged commit 56cd36b into master May 31, 2017
@kortschak kortschak deleted the extraction branch June 3, 2017 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants