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

Minres solver #975

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Minres solver #975

wants to merge 3 commits into from

Conversation

MarcelKoch
Copy link
Member

@MarcelKoch MarcelKoch commented Feb 22, 2022

This PR will add a Minres solver for symmetric/hermitian indefinite matrices. The implementation is based in the book 'Iterative Methods for Solving Linear Systems' (Ch. 8) by Anne Greenbaum (DOI: https://doi.org/10.1137/1.9781611970937). It uses the common kernel interface to define kernels.

One uncertainty for me is the computation of the residual norm approximation. I'm using the approach from Iterative Methods for Singular Linear Equations and Least-Squares Problems, but somehow this does not define the initial value for the used recursion. The three possible values are beta = sqrt(<r, z>), ||r|| or ||z||. I'm currently using ||z||, since I could also find that used by petsc. I've compared these three in matlab for smaller matrices, and the difference between these does not seem to matter.

Todo:

@MarcelKoch MarcelKoch added is:new-feature A request or implementation of a feature that does not exist yet. type:solver This is related to the solvers 1:ST:ready-for-review This PR is ready for review mod:all This touches all Ginkgo modules. labels Feb 22, 2022
@MarcelKoch MarcelKoch added this to the Ginkgo 1.5.0 milestone Feb 22, 2022
@MarcelKoch MarcelKoch self-assigned this Feb 22, 2022
@ginkgo-bot ginkgo-bot added mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:testing This is related to testing. labels Feb 22, 2022
@MarcelKoch
Copy link
Member Author

format-rebase!

@ginkgo-bot
Copy link
Member

Error: Rebase failed, see the related Action for details

@MarcelKoch
Copy link
Member Author

format!

@MarcelKoch MarcelKoch force-pushed the minres-solver branch 4 times, most recently from 26a67da to ae95b88 Compare February 23, 2022 08:56
@MarcelKoch MarcelKoch requested a review from a team February 23, 2022 08:57
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell B 160 Code Smells

72.8% 72.8% Coverage
13.0% 13.0% Duplication

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (1e268ff) 91.03% compared to head (6a991bc) 91.72%.
Report is 4 commits behind head on develop.

❗ Current head 6a991bc differs from pull request most recent head 7e34ec9. Consider uploading reports for the commit 7e34ec9 to get more accurate results

Files Patch % Lines
core/solver/minres.cpp 73.56% 23 Missing ⚠️
common/unified/solver/minres_kernels.cpp 83.72% 7 Missing ⚠️
include/ginkgo/core/solver/minres.hpp 64.28% 5 Missing ⚠️
core/device_hooks/common_kernels.inc.cpp 0.00% 3 Missing ⚠️
reference/test/solver/minres_kernels.cpp 97.72% 3 Missing ⚠️
reference/solver/minres_kernels.cpp 96.42% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #975      +/-   ##
===========================================
+ Coverage    91.03%   91.72%   +0.69%     
===========================================
  Files          697      503     -194     
  Lines        56724    43056   -13668     
===========================================
- Hits         51639    39494   -12145     
+ Misses        5085     3562    -1523     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

"A comma-separated list of solvers to run. "
"Supported values are: bicgstab, bicg, cb_gmres_keep, "
"cb_gmres_reduce1, cb_gmres_reduce2, cb_gmres_integer, "
"cb_gmres_ireduce1, cb_gmres_ireduce2, cg, cgs, fcg, minres, gmres, idr, "
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we try to use alphabetical order here, or?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I guess I wasn't paying attention.

@upsj upsj self-requested a review March 17, 2022 11:12
@Slaedr Slaedr self-requested a review March 17, 2022 11:12
@upsj upsj removed this from the Ginkgo 1.5.0 milestone Apr 20, 2022
@MarcelKoch MarcelKoch added the 1:ST:WIP This PR is a work in progress. Not ready for review. label May 19, 2022
@Slaedr
Copy link
Contributor

Slaedr commented May 31, 2022

I've realized that there are issues if the jacobi preconditioner is used for matrices with negative diagonal elements. I need to further investigate this, so I will put the PR back to WIP.

In general, Jacobi is not supposed to work for indefinite systems. Is the system indefinite? If so, this may not be an issue. If the matrix is diagonal dominant, but has both negative and positive diagonal elements, I guess there's a high chance it's indefinite.

@MarcelKoch MarcelKoch force-pushed the minres-solver branch 2 times, most recently from 285d451 to 2af34a1 Compare December 15, 2023 08:49
@MarcelKoch MarcelKoch added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Dec 15, 2023
@MarcelKoch MarcelKoch requested a review from a team December 15, 2023 08:52
@MarcelKoch MarcelKoch force-pushed the minres-solver branch 4 times, most recently from 777448c to 7e34ec9 Compare December 15, 2023 11:03
@MarcelKoch MarcelKoch added this to the Ginkgo 1.9.0 milestone Aug 26, 2024
@MarcelKoch MarcelKoch modified the milestones: Ginkgo 1.9.0, Ginkgo 1.10.0 Dec 9, 2024
@MarcelKoch MarcelKoch requested a review from venkovic February 13, 2025 13:26
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

I would like to see a mention of the used algorithm in the header description as well (the details can remain in the CPP file).
The rest looks good; my other comments are primarily nits.
Note: This review was done together with @nvenko, so consider our two reviews as one.

Copy link

@venkovic venkovic left a comment

Choose a reason for hiding this comment

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

The two used bibliographic references can be added in the minres header file. Also, the namespace should be corrected in the minres_kernels header file, from cg to minres.

Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

LGTM!

MarcelKoch and others added 3 commits February 19, 2025 15:35
based on 'Iterative Methods for Solving Linear Systems' (https://doi.org/10.1137/1.9781611970937) and 'Iterative Methods for Singular Linear Equations and Least-Squares Problems' (PhD thesis, Stanford University)

Co-authored-by: Aditya Kashi <aditya.kashi@kit.edu>
Co-authored-by: Hartwig Anzt <hanzt@icl.utk.edu>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
- documentation
- missing tests
- refactor

Co-authored-by: Thomas Grützmacher <thomas.gruetzmacher@tum.de>
Co-authored-by: nvenko <venkovic@gmail.com>
Signed-off-by: Marcel Koch <marcel.koch@kit.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review is:new-feature A request or implementation of a feature that does not exist yet. mod:all This touches all Ginkgo modules. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. mod:reference This is related to the reference module. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:testing This is related to testing. type:solver This is related to the solvers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants