Refactor preconditioner implementation #167
Conversation
+ use coarse solver in schwarz, clean-up
+ format
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the preconditioner implementation by extracting the generation logic from a monolithic class into separate, focused header files. The main changes involve moving preconditioner-specific code into dedicated classes while introducing a centralized coarse solver generation routine for better code organization and maintainability.
- Extracted preconditioner implementations into dedicated header files (Jacobi, LU, Cholesky, ISAI, Multigrid)
- Added a centralized coarse solver generation function in CoarseSolver.cpp
- Introduced utility functions for Schwarz wrapping operations
Reviewed Changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Preconditioner/CoarseSolver.cpp | New implementation for centralized coarse solver generation |
| include/OGL/StoppingCriterion.hpp | Minor formatting improvement to debug message |
| include/OGL/Preconditioner/Schwarz.hpp | New utility functions for wrapping preconditioners with Schwarz methods |
| include/OGL/Preconditioner/PreconditionerWrapper.hpp | New base wrapper class definition |
| include/OGL/Preconditioner/Multigrid.hpp | New dedicated Multigrid preconditioner class |
| include/OGL/Preconditioner/LU.hpp | New LU factorization preconditioner class |
| include/OGL/Preconditioner/Jacobi.hpp | New Block Jacobi preconditioner class |
| include/OGL/Preconditioner/ISAI.hpp | New ISAI preconditioner class |
| include/OGL/Preconditioner/CoarseSolver.hpp | Header declaration for coarse solver generation function |
| include/OGL/Preconditioner/Cholesky.hpp | New Cholesky factorization preconditioner class |
| include/OGL/Preconditioner.hpp | Refactored main class to use new dedicated preconditioner classes |
| CMakeLists.txt | Added new source file to build configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (solver == "CG") { | ||
| coarsest_solver = gko::share(cg::build() | ||
| .with_preconditioner(bjfac) | ||
| .with_criteria(solve_it, solve_red) | ||
| .on(exec)); | ||
| } | ||
| if (solver == "BiCGStab") { | ||
| coarsest_solver = gko::share(bicgstab::build() | ||
| .with_preconditioner(bjfac) | ||
| .with_criteria(solve_it, solve_red) | ||
| .on(exec)); | ||
| } | ||
| if (solver == "GMRES") { | ||
| coarsest_solver = gko::share(gmres::build() | ||
| .with_preconditioner(bjfac) | ||
| .with_criteria(solve_it, solve_red) | ||
| .on(exec)); | ||
| } | ||
| if (solver == "Jacobi") { | ||
| scalar relaxFac{ | ||
| solverDict.lookupOrDefault("relaxationFactor", scalar(0.9))}; | ||
| coarsest_solver = gko::share(ir::build() | ||
| .with_solver(bjfac) | ||
| .with_relaxation_factor(relaxFac) | ||
| .with_criteria(solve_it, solve_red) | ||
| .on(exec)); | ||
| } |
There was a problem hiding this comment.
This chain of if statements should use else-if to avoid unnecessary condition checks once a match is found. The current implementation continues checking all conditions even after finding a match.
Copilot uses AI. Check for mistakes.
| auto coarsening = d.lookupOrDefault<word>("coarsening", word("PGM")); | ||
| auto coarseWeight = d.lookupOrDefault("coarseWeight", scalar(0.1)); | ||
|
|
||
| if (coarsening == "fixed") { |
There was a problem hiding this comment.
This function has multiple return paths and incomplete logic. The function can return from either the 'fixed' or 'PGM' coarsening branches, but there's no return statement at the end for unhandled cases, which could lead to undefined behavior.
Copilot uses AI. Check for mistakes.
| std::shared_ptr<gko::LinOp> generate_factorization() const | ||
| { | ||
| if (factorization_ == "ILU") { |
There was a problem hiding this comment.
The function generate_factorization() has multiple return paths through if statements but no return statement at the end, which could lead to undefined behavior if none of the conditions are met.
Copilot uses AI. Check for mistakes.
| virtual std::shared_ptr<gko::LinOp> create() | ||
| { | ||
| if (type_ == "SPD") { | ||
| return dispatch_schwarz(mtx_, exec_, generate_precond_factory_spd(), | ||
| d_, verbose_); | ||
| } | ||
| if (type_ == "General") { | ||
| return dispatch_schwarz( | ||
| mtx_, exec_, generate_precond_factory_general(), d_, verbose_); | ||
| } | ||
| } |
There was a problem hiding this comment.
The function has multiple return paths but no return statement at the end, which could lead to undefined behavior if none of the type conditions are met.
Copilot uses AI. Check for mistakes.
| std::shared_ptr<gko::LinOp> generate_factorization() const | ||
| { | ||
| if (factorization_ == "IC") { |
There was a problem hiding this comment.
The function generate_factorization() has multiple return paths through if statements but no return statement at the end, which could lead to undefined behavior if none of the conditions are met.
Copilot uses AI. Check for mistakes.
greole
force-pushed
the
refact/precond
branch
from
9416a48 to
f29c729
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
greole
force-pushed
the
enh/repart_comm
branch
from
82efe8e to
28ee971
Compare
This PR refactors the preconditioner implementation by placing the generation into separate files. Additionally, a central coarse solver generation routine is added.