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

Major refactoring of the codebase. #92

Merged
merged 33 commits into from
Jun 13, 2024
Merged

Major refactoring of the codebase. #92

merged 33 commits into from
Jun 13, 2024

Conversation

Simkern
Copy link
Collaborator

@Simkern Simkern commented Jun 12, 2024

  • innerprod, deployed throughout
  • orthogonalize_against_basis, deployed throughout the toolbox.
  • one_*/zero_* -> LightKrylov_Constants
  • Improved logging
  • Streamlined testing to avoid multiple solver calls.
  • Added test for eigen/singular values and associated vectors for iterative eigen/svd solvers
  • Homogenization of the files.

@Simkern Simkern requested a review from loiseaujc June 12, 2024 17:23
Copy link
Member

@loiseaujc loiseaujc left a comment

Choose a reason for hiding this comment

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

It looks pretty good to me. The only concern I have is about the orthogonalize_vector_against_basis and its extension. I feel like these should actually implement a proper two-pass modified Gram-Schmit process. We wouldn't be re-using the innerprod function, but at least the implementation will be numerically stable for sure. Moreover, it would enable us to get rid of the second passes in all the functions which rely on Gram-Schmidt (e.g. qr, gmres, update_hessenberg, etc.)

src/BaseKrylov.fypp Outdated Show resolved Hide resolved
src/BaseKrylov.fypp Show resolved Hide resolved
…ize_against_basis and for double_gram_schmidt_step.
    Added double_gram_schmidt_step and deployed it with logging.
    Replaces update_hessenberg_matrix.
    Added optional orthonormality check in orthogonalize_against_basis.
    Set this check to false in all performance-critical routines in the core.
IterativeSolvers.f90:
    Deployed double_gram_schmidt_step.
    Deployed orthonormalize_against_basis.
@Simkern
Copy link
Collaborator Author

Simkern commented Jun 13, 2024

The only concern I have is about the orthogonalize_vector_against_basis and its extension. I feel like these should actually implement a proper two-pass modified Gram-Schmit process. We wouldn't be re-using the innerprod function, but at least the implementation will be numerically stable for sure. Moreover, it would enable us to get rid of the second passes in all the functions which rely on Gram-Schmidt (e.g. qr, gmres, update_hessenberg, etc.)

I haven't checked in on the community recently, but I remember there being some controversy regarding the stability of MGS for some fringe cases and some people even advocating for triple GS ... In any case, MGS is more expensive than DGS, while the latter usually does the trick as well in my experience. I've now gone with DGS, which is a wrapper and makes the orthogonalization fairly clean throughout the code.

@loiseaujc loiseaujc merged commit 3c447b1 into dev Jun 13, 2024
1 check passed
@loiseaujc loiseaujc deleted the add_tools branch June 13, 2024 12:28
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.

2 participants