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

Make orthogonalization method chooseable for gmres #293

Merged
merged 8 commits into from
Mar 19, 2021

Conversation

jkrch
Copy link
Contributor

@jkrch jkrch commented Mar 19, 2021

When using multithreading, Classical Gram-Schmidt with and without the DGKS correction step provides better runtimes then Modified Gram-Schmidt (since they use BLAS level 2 instead of level 1).
The following shows GMRES solving the Bourchtein/atmosmodd linear system from SuiteSparse using the first from the two RHS which come with the matrix. All three runs converged after 1497 iterations (so at least for this linear system round off errors doesn't seem to be a problem).
orth_meth

Since CGS and DGKS are already implemented, I just made them choosable when calling gmres. Set MGS the default, since it might be the best without multithreading.

@coveralls
Copy link

coveralls commented Mar 19, 2021

Pull Request Test Coverage Report for Build 669141589

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 97.773%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/orthogonalize.jl 0 1 0.0%
Totals Coverage Status
Change from base Build 667897591: 0.2%
Covered Lines: 1932
Relevant Lines: 1976

💛 - Coveralls

src/gmres.jl Outdated Show resolved Hide resolved
src/gmres.jl Outdated Show resolved Hide resolved
Copy link
Member

@haampie haampie left a comment

Choose a reason for hiding this comment

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

I think modified Gram-Schmidt is what people expect. I don't have the paper but I believe it should be dominated by rounding errors only exactly upon convergence, whereas classical GS can be dominated by noise before convergence.

Co-authored-by: Harmen Stoppels <[email protected]>
@haampie
Copy link
Member

haampie commented Mar 19, 2021

Can you include some comments explaining in not too technical terms what the user can choose and why he/she would want to do that? Basically comment on accuracy vs performance.

jkrch added 2 commits March 19, 2021 18:17
Removed the type specification for orth_meth since it resulted in an error and I do not know how to resolve the error. Exporting OrthogonalizationMethod in orthogonalize.jl did not help.
@jkrch
Copy link
Contributor Author

jkrch commented Mar 19, 2021

I can't get it running with orth_meth::OrthogonalizationMethod = ModifiedGramSchmidt :/
Thougth exporting OrthogonalizationMethod in orthogonalize.jl would help, but didn't...

@haampie
Copy link
Member

haampie commented Mar 19, 2021

Oh, you need ::Type{<:OrthogonalizationMethod} when passing the type along instead of an instance of it.

I'm also fine if you replace all occurances of

orthogonalize_and_normalize!(..., ::Type{DGKS}) with orthogonalize_and_normalize!(..., ::DGKS)

and make the user pass DGKS(), and the same for the other types

@jkrch
Copy link
Contributor Author

jkrch commented Mar 19, 2021

Ah ok, gonna replace it

docs/src/linear_systems/gmres.md Outdated Show resolved Hide resolved
Co-authored-by: Harmen Stoppels <[email protected]>
@haampie haampie merged commit ae01dfe into JuliaLinearAlgebra:master Mar 19, 2021
@haampie
Copy link
Member

haampie commented Mar 19, 2021

Thanks for your contribution!

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.

3 participants