-
Notifications
You must be signed in to change notification settings - Fork 60
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
Reworked upper Hessenberg decomposition #179
Open
Andlon
wants to merge
10
commits into
AtheMathmo:master
Choose a base branch
from
Andlon:hessenberg
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Necessary for testing the validity of the output from Hessenberg decomposition.
We don't need the buffer to be a specific size, it's sufficient that it's large enough to hold the intermediate data. This simplifies usage a little, since we don't need to truncate or resize the buffer when it is reused for different instances of HouseholderReflection.
The composition of Householder matrices for QR decomposition and Upper Hessenberg decomposition is very similar. We can exploit this to reuse virtually all code we have written for HouseholderComposition in the QR decomposition to implement the Hessenberg decomposition. We do this by adding a "subdiagonal" parameter to HouseholderComposition which controls the subdiagonal to pull Householder reflectors from.
Andlon
commented
May 7, 2017
|
||
for element in y.iter_mut() { | ||
*element = T::zero(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This zeroing isn't actually necessary! Should remove
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR continues the trend of reworking our decompositions one-by-one. Because the Hessenberg decomposition is very similar to the QR decomposition, we are able to reuse a large amount of functionality. In order to do this, we must extend the concept of
HouseholderComposition
to also allow Householder transformations that operate on the subdiagonal of the matrix rather than the main diagonal (which correspond to QR).I recommend to look through the commit messages for an overview of how this PR is structured and what changes it makes to existing infrastructure.
Some discussion points
HessenbergDecomposition
. Would it be sufficient to call itHessenberg
? I suppose havingDecomposition
in the name makes it explicitly clear that we are talking about the decomposition. It's strictly speaking an upper Hessenberg decomposition, but I've never even heard of anyone using a lower Hessenberg decomposition, so I think this is OK - the documentation makes it clear that we are talking about an upper Hessenberg decomposition.HouseholderComposition::left_multiply_into
whensubdiagonal == 1
(corresponding to Hessenberg decomposition). These tests are rather painful to write due to the need for generating test data. The current test only tests forsubdiagonal == 0
(corresponding to QR). However, I think this is OK because the Hessenberg tests verify the correctness of the Hessenberg decomposition, which implicitly depends on the correctness ofleft_multiply_into
, as well asfirst_k_columns
.Performance
Benchmarks show quite considerably increased performance compared to the existing hessenberg routines:
In the above,
hessenberg_decomposition_*
corresponds to the new functionality. The newdecompose_NxN
is comparable in functionality to the oldupper_hessenberg
function, anddecompose_unpack_NxN
is comparable to the oldupper_hess_decomp
function.