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

Change the QR implementation, inspired by LAPACK #730

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jannschu
Copy link

@jannschu jannschu commented May 9, 2020

I ported some ideas from LAPACK's QR implementation, more specifically from ?ORG2R.

As a result I could increase all epsilons in the test by two order of magnitudes. Also the full computation of the Q matrix is now supported, not only the first min(n, m) columns. I added a new method q_columns for this. The current q() method stayed the same.

The main difference in the implementation seems to be that LAPACK applies the scaling of the Householder vectors at a different point in the code.

Breaking changes

  • The internal representation has changed (is exposed by hidden method qr_internal)
  • For some inputs some column signs flipped of R and Q.
  • Some allocations were added, can be found when searching for work. This resulted in additional allocator bounds. See discussion below.

Things to discuss

  • Currently q_tr_mul borrows self mutably due to an implementation detail although the struct's state will only be changed temporarily inside the method. Ideas to resolve this:
    • Keep as is
    • Use interior mutability
    • Allocate
    • Change affected gerc and gemv call (possible performance penalty)
  • This would be a good occasion to implement FullPivLU and QR refuse to solve for non square matrix #672 for QR
  • The additional work allocations could be done only once by storing a suitable vector in the struct and sharing it. This will result in &mut self or interior mutability.

@sebcrozet sebcrozet force-pushed the dev branch 3 times, most recently from 700c4dc to c5c6c13 Compare April 12, 2021 14:15
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.

1 participant