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

fix LAPACK.ormlq! and postmultiplication with LQPackedQ #23803

Merged
merged 2 commits into from
Sep 21, 2017

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Sep 20, 2017

This pull request fixes LAPACK.ormlq!'s shape checks, consequently fixing the bug in postmultiplication with LQPackedQs identified in JuliaLang/LinearAlgebra.jl#469. This pull request also makes LQPackedQ's context-dependent postmultiplication behavior (see #23785 (comment)) consistent across all existing out-of-place multiplication methods (*, A[c]_mul_B[c]) (edit: botched) and tests those methods' behavior. Best!

(The first commit is #23785, which I will rebase out.)

@Sacha0 Sacha0 added bugfix This change fixes an existing bug linear algebra Linear algebra labels Sep 20, 2017
Copy link
Member Author

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

The A_mul_Bc(C, Q) and Ac_mul_Bc(C, Q) methods in this pull request do not require LQPackedQ's context-dependent multiplication behavior as LQPackedQ's second dimension is not context-dependent. Revisions inbound tomorrow. Thanks!

# the number of rows of the matrix of which Q is a factor (i.e. size(Q.factors, 1)),
# and if so zero-extends A as necessary for check (1) to pass (if possible).
*(A::StridedVecOrMat, Q::LQPackedQ) = _A_mul_Bq(A_mul_B!, A, Q)
A_mul_Bc(A::StridedVecOrMat, Q::LQPackedQ) = _A_mul_Bq(A_mul_Bc!, A, Q)
Copy link
Member Author

Choose a reason for hiding this comment

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

This method does not require LQPackedQ's context-dependent multiplication behavior, as LQPackedQ's second dimension is not context-dependent.

return A_mul_Bop!(C, convert(AbstractMatrix{TR}, Q))
end
Ac_mul_B(A::StridedMatrix, Q::LQPackedQ) = _Ac_mul_Bq(A_mul_B!, A, Q)
Ac_mul_Bc(A::StridedMatrix, Q::LQPackedQ) = _Ac_mul_Bq(A_mul_Bc!, A, Q)
Copy link
Member Author

Choose a reason for hiding this comment

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

This method also does not require LQPackedQ's context-dependent multiplication behavior, as LQPackedQ's second dimension is not context-dependent.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Great work and thanks for adding all the comments.

@andreasnoack andreasnoack merged commit 1a3a633 into JuliaLang:master Sep 21, 2017
@Sacha0
Copy link
Member Author

Sacha0 commented Sep 21, 2017

Thanks Andreas! I will prepare a followup pull request with the touchups mentioned above :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants