Refactor preconditioner implementation by greole · Pull Request #167 · hpsim/OGL

greole · 2025-09-15T09:42:48Z

This PR refactors the preconditioner implementation by placing the generation into separate files. Additionally, a central coarse solver generation routine is added.

Copilot

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.}

Copilot · 2025-09-17T09:03:24Z

+    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));
+    }


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 · 2025-09-17T09:03:25Z

+    auto coarsening = d.lookupOrDefault<word>("coarsening", word("PGM"));
+    auto coarseWeight = d.lookupOrDefault("coarseWeight", scalar(0.1));
+
+    if (coarsening == "fixed") {


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 · 2025-09-17T09:03:26Z

+    std::shared_ptr<gko::LinOp> generate_factorization() const
+    {
+        if (factorization_ == "ILU") {


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 · 2025-09-17T09:03:27Z

+    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_);
+        }
+    }


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 · 2025-09-17T09:03:27Z

+    std::shared_ptr<gko::LinOp> generate_factorization() const
+    {
+        if (factorization_ == "IC") {


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.

…update

+ use coarse solver in schwarz, clean-up

+ format

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…update

Copilot

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 21 comments.

+        word frequencyMode = "optimizer";
        label minIter = minIter_;
        label frequency = frequency_;
+        // in case of export_res all residuals need to be computed
        if (!export_res) {
            if (prev_solve_iters > 0 && adapt_minIter_ && prev_rel_cost > 0) {
                minIter = prev_solve_iters * relaxationFactor_;
-                auto alpha =
-                    sqrt(1.0 / (prev_solve_iters * (1.0 - relaxationFactor_)) *
-                         prev_rel_cost);
-                frequency = min(norm_eval_limit_, max(1, label(1 / alpha)));
+                if (frequencyMode == "optimizer") {
+                    auto alpha = sqrt(
+                        1.0 / (prev_solve_iters * (1.0 - relaxationFactor_)) *
+                        prev_rel_cost);
+                    frequency = min(norm_eval_limit_, max(1, label(1 / alpha)));
+                }
+                if (frequencyMode == "relative") {
+                    frequency = label(prev_solve_iters * 0.075) + 1;


-                   std::to_string(frequency);
+                   std::to_string(frequency) + " prev_solve_iters " +
+                   std::to_string(prev_solve_iters) + " adapt_minIter_  " +
+                   std::to_string(adapt_minIter_) + " prev_rel_cost  ";


+    auto start_axref = std::chrono::steady_clock::now();
    compute_Axref_dist(global_size[0], local_size[0], device_exec, gkomatrix, x,
                       Axref);
-
+    auto end_axref = std::chrono::steady_clock::now();
+    auto delta_t_axref = std::chrono::duration_cast<std::chrono::microseconds>(
+                             end_axref - start_axref)
+                             .count() /
+                         1.0;


+                                        const AllToAllPattern allToAllIn,
+                                        label start_rank)
+{
+    auto host_comm = exec_handler.get_host_comm();


+    send_offsets.back() = scalar(allToAllIn.send_offsets.back());
+    recv_offsets.back() = scalar(allToAllIn.recv_offsets.back());


+        if (factorization_ == "ParILU") {
+            label iterations = d_.lookupOrDefault("iterations", label(5));
+            auto factorization_factory =
+                gko::factorization::ParIlut<scalar, label>::build()
+                    .with_skip_sorting(skip_sorting_)
+                    .with_iterations(iterations)
+                    .on(exec_);
+


+          type_(d.lookupOrDefault("type", word("general"))),
+          sparsityPower_(d.lookupOrDefault("sparsityPower", label(1)))
+    {
+        word msg = "Generate " + type_ + "ISAI" +
+                   "  preconditioner:\n\tsparsityPower: " +
+                   std::to_string(sparsityPower_);
+        MLOG_0(verbose_, msg)
+    }
+
+    auto generate_precond_factory_spd()
+    {
+        return gko::preconditioner::Isai<gko::preconditioner::isai_type::spd,
+                                         scalar, label>::build()
+            .with_skip_sorting(skip_sorting_)
+            .with_sparsity_power(sparsityPower_)
+            .on(exec_);
+    }
+
+    auto generate_precond_factory_general()
+    {
+        return gko::preconditioner::Isai<
+                   gko::preconditioner::isai_type::general, scalar,
+                   label>::build()
+            .with_skip_sorting(skip_sorting_)
+            .with_sparsity_power(sparsityPower_)
+            .on(exec_);
+    }
+
+    virtual std::shared_ptr<gko::LinOp> create()
+    {
+        if (type_ == "SPD") {
+            auto b = generate_precond_factory_spd();
+            return dispatch_schwarz(
+                mtx_, exec_,
+                gko::share(
+                    b->generate(gko::as<RepartDistMatrix>(mtx_)->get_local())),
+                d_, verbose_);
+        }
+        if (type_ == "General") {
+            auto b = generate_precond_factory_general();
+            return dispatch_schwarz(


+/* @brief computes AllToAllPattern for repartioned communincator from global
+ * allToAll pattern by discarding all zero communication before and after the
+ * repartioner scope.


+    // NOTE in case of symmetric matrix 0 (upper) is same as 1 (lower)
+    // thus we can start at 1


+        auto solveNormC =
+            d.lookupOrDefault("reductionCoarseSolver", label(1e-6));
+        auto maxIterCoarse = d.lookupOrDefault("maxIterCoarse", label(50));


greole changed the base branch from dev to stack/ogl_0.6 September 15, 2025 09:43

greole changed the base branch from stack/ogl_0.6 to enh/repart_comm September 15, 2025 09:43

greole changed the title ~~Refact/precond~~ Refactor preconditioner implementation Sep 17, 2025

greole requested a review from Copilot September 17, 2025 09:00

Copilot AI reviewed Sep 17, 2025

View reviewed changes

greole force-pushed the refact/precond branch from 9416a48 to f29c729 Compare September 17, 2025 09:04

greole force-pushed the enh/repart_comm branch from 82efe8e to 28ee971 Compare September 25, 2025 15:16

greole force-pushed the refact/precond branch from 78fea9e to dc72d76 Compare October 30, 2025 13:56

greole changed the base branch from enh/repart_comm to dev May 7, 2026 16:02

greole and others added 21 commits May 9, 2026 11:43

use repartAllToAll

604c805

add get_ranks_per_gpu

f9d9a99

add basic implementation of gatherv instead off alltoallv for repart …

e0041d2

…update

use forceHostBuffer for vector updates

b4cadf5

wip performance tuning

d0fe5a1

format files

d0c11e9

use optimizer as default

aa977a1

WIP, avoid symmetric MPI_Igatherv

11509bc

wip move preconditioner to separate implementation files

63cec6f

move LU implementation

af2b40c

format

6456a6c

add ISAI

fafcc9e

clean-up

597d5d4

move to CoarseSolver.cpp

6924ad6

+ use coarse solver in schwarz, clean-up

extract num rows in ml schwarz

21385a2

+ format

use schwarz dispatch function

941bccd

Apply suggestions from code review

ea03809

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

clean-up some unneeded gko::share

bd2c1e2

add basic implementation of gatherv instead off alltoallv for repart …

0c3fae9

…update

use forceHostBuffer for vector updates

350fcff

performance tuning

8fb475d

greole added 3 commits May 9, 2026 12:00

use optimizer as default

08c593a

Avoid symmetric MPI_Igatherv

9886bc5

fixup

723e510

greole force-pushed the refact/precond branch from 8e737df to 723e510 Compare May 9, 2026 10:06

greole added 2 commits May 9, 2026 12:23

fixup!

5b164b1

fixup!

73516e4

greole requested a review from Copilot May 9, 2026 13:30

Copilot started reviewing on behalf of greole May 9, 2026 13:30 View session

Copilot AI reviewed May 9, 2026

View reviewed changes

greole and others added 4 commits May 10, 2026 21:19

wip fix distmultigrid

9a05779

fixup

15d6094

wip fix distmultigrid

9a5a7d6

wip fix distmultigrid

047c4c0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor preconditioner implementation#167

Refactor preconditioner implementation#167
greole wants to merge 30 commits into
devfrom
refact/precond

greole commented Sep 15, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 17, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Uh oh!

Copilot AI Sep 17, 2025

Uh oh!

Copilot AI Sep 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		send_offsets.back() = scalar(allToAllIn.send_offsets.back());
		recv_offsets.back() = scalar(allToAllIn.recv_offsets.back());

		// NOTE in case of symmetric matrix 0 (upper) is same as 1 (lower)
		// thus we can start at 1

Conversation

greole commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greole commented Sep 15, 2025 •

edited

Loading