Skip to content

SNES solver: Added a PID controller to update the timestep #3146

Merged
bendudson merged 16 commits into
nextfrom
snes-adaptive-pid
Aug 12, 2025
Merged

SNES solver: Added a PID controller to update the timestep #3146
bendudson merged 16 commits into
nextfrom
snes-adaptive-pid

Conversation

@malamast
Copy link
Copy Markdown
Contributor

@malamast malamast commented Jul 3, 2025

The user sets target_its a desired number of nonlinear iterations and the timestep is updated based on the ratio of that number and the actual number on nonlinear iteration of each Newton step. The exponent kI can be tuned to the problem needs. A smaller exponent results in smaller changes of dt.

It has been tested with the 1D-threshold example and it gave a small speed up over the old method. There is good agreement between the different solvers for most of the variables. It seems that velocities are more sensitive to the accuracy of the solver. See also the attached figures where I compare the solution at the last timestep. Some figures show the time evolution of the variables at the middle of the 1D domain.

I have also added an option to let the user set the maximum number of function evaluations maxf . One might need to increase maxf, if large values of output_step are set.

How to use:
nout = 50
timestep = 95788 * 0.05 # 95788 = 1 milisecond

[solver]
type = beuler # Backward Euler steady-state solver
snes_type = newtonls # Newton iterations
ksp_type = gmres # GMRES method
pc_type = hypre # Preconditioner type
pc_hypre_type = euclid # Hypre preconditioner type
max_nonlinear_iterations = 16 # Max Newton iterations per timestep
lag_jacobian = 6 # How long to wait before Jacobian recalculation per Newton iteration

atol = 1e-12 # Absolute tolerance
rtol = 1e-6 # Relative tolerance
stol = 1e-10 # Convergence tolerance
maxf = 20000 # Maximum number of function evaluations
maxl = 120 # default: 20
#diagnose = true
#diagnose_failures = true

pidController = true
target_its = 5
kP = 0.7
kI = 0.3
kD = 0.2

Nd+_vs_cellNum
Td+_vs_cellNum
Ne_vs_t
Td+_vs_t
Ve_vs_cellNum

…kward euler SNES solver.

The user sets target_its a desired number of nonlinear iterations and the timestep is updated based on the ratio of that number and the actual number on nonlinear iteration of each Newton step. The exponent can be tuned to the problem needs. A smaller exponent results in smaller changes of dt.
@malamast malamast requested a review from bendudson July 3, 2025 02:27
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
@boutproject boutproject deleted a comment from github-actions Bot Jul 3, 2025
Copy link
Copy Markdown
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

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

I've deleted the clang-tidy comments as the clang-format commit rearranged things and confused it, and put them back on the right places.

It would be nice to get a test on this. We have an integrated test for all the solvers, but it's not currently set up to handle different options on the same solver. I think I have an idea on how to handle that -- but it might be best to do that in a separate PR?

Comment thread src/solver/impls/snes/snes.cxx Outdated
Comment thread src/solver/impls/snes/snes.cxx Outdated
Comment thread src/solver/impls/snes/snes.cxx Outdated
Comment thread src/solver/impls/snes/snes.cxx Outdated
Comment thread src/solver/impls/snes/snes.cxx Outdated
Comment thread src/solver/impls/snes/snes.cxx Outdated
@mikekryjak
Copy link
Copy Markdown
Contributor

@malamast you need to run the simulations for longer - 50 timesteps is not enough to reach full steady state. It was chosen in the example to keep the solution time reasonable for both CVODE and SNES simultaneously. Maybe 200 timesteps would be enough to get all of the oscillations out for sure, and should hopefully result in all three converging to the same profiles.

The main question is how is the time evolution affected. It would be good to have plots of upstream density and target temperature as a function of time if you can. Your results so far definitely show that there are some differences - this is fine as we want a fast steady state solution, but it would be good to quantify it a bit.

Also, what was your motivation for implementing the maxf setting? Is it to prevent the solver from getting stuck in some new way?

P.S. it makes sense that velocity is the most different. The plasma sloshes around left and right for a while because this example doesn't have viscosity enabled.

malamast and others added 8 commits July 7, 2025 13:10
Code changes suggested by ZedThree

Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
@malamast
Copy link
Copy Markdown
Contributor Author

malamast commented Jul 9, 2025

I ran the example for longer and the two solvers converge to the same solution. See attached figures.
Run time on 4 CPUs:
CVODE: 12 h 22 m 42 s
SNES: 2 m 37 s

Input settings:

type = beuler # Backward Euler steady-state solver
snes_type = newtonls # Newton iterations: newtonls
ksp_type = gmres # GMRES method
pc_type = hypre # Preconditioner type
pc_hypre_type = ilu # Hypre preconditioner type
max_nonlinear_iterations = 16 # Max Newton iterations per timestep
lag_jacobian = 6 # How long to wait before Jacobian recalculation per Newton iteration
atol = 1e-12 # Absolute tolerance
rtol = 1e-6 # Relative tolerance
stol = 1e-10 # Convergence tolerance
maxf = 20000 # Maximum number of function evaluations
#timestep = 1.0 # Initial timestep
maxl = 120 # default: 20
diagnose = true
matrix_free_operator = true
pidController = true
target_its = 5
kP = 0.65
kI = 0.25
kD = 0.15

Nd+_vs_cellNum
Nd_vs_cellNum
Nd+_vs_t
Te_vs_cellNum
Te_vs_t
Ve_vs_cellNum
Ve_vs_t

Comment thread src/solver/impls/snes/snes.cxx Outdated
@ZedThree
Copy link
Copy Markdown
Member

Run time on 4 CPUs:
CVODE: 12 h 22 m 42 s
SNES: 2 m 37 s

That's seriously impressive! How does non-PID SNES fair?

Please could you plot these quantities on a log-y scale, and show the R^2?

malamast and others added 2 commits July 17, 2025 09:56
… added the line MatSetOption(Jfd,MAT_KEEP_NONZERO_PATTERN,PETSC_TRUE); after the creation of Jfd to tell petsc to keep the pattern of the non zero elements in memory. Also, I changed the default coloring type to greedy which is more suited for large parallel problems.
@malamast
Copy link
Copy Markdown
Contributor Author

Here are more plots in log-y scale.

Run time on 4 CPUs:
non-PID = 6 m 45 s
with PID = 4 m 30 s

I think the main reason we see a huge speed-up we see with SNES vs CVODE is that SNES uses hypre preconditioners and finite difference jacobian with coloring. Also, SNES sets a convergence tolerance stol, which allows the solver to take large timesteps as the solution appraches a steady state. CVODE was forced to take small steps even at steady state (although, to be fair, I used rtol = 10^-6 for both solvers and CVODE can be fast if one relaxes the tolerance.)

Also, a big speed-up with SNES is achieved by setting:
matrix_free_operator = true

Nd+_vs_cellNum
Nd_vs_cellNum
Nd+_vs_t
Nd_vs_t
Ne_vs_cellNum
Ne_vs_t
Td+_vs_cellNum
Td_vs_cellNum
Td+_vs_t
Td_vs_t
Te_vs_cellNum
Te_vs_t

@mikekryjak
Copy link
Copy Markdown
Contributor

Ok, so a nice 30% performance speedup thanks to the PID controller. Great work! Wonder if tuning can improve this further.
When you enable matrix_free, doesn't that mean we cannot use an algebraic preconditioner because no matrix is being assembled (like we do in CVODE)? I would have thought it would be much slower...

@malamast
Copy link
Copy Markdown
Contributor Author

Yes, matrix_free should be set to false for these transport cases. From what I discussed with Ben, matrix_free_operator is relevant for GMRES and leads to a performance improvement in both the 1D and 2D cases.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/solver/impls/snes/snes.cxx Outdated

// Set size of Matrix on each processor to nlocal x nlocal
MatCreate(BoutComm::get(), &Jfd);
MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MAT_KEEP_NONZERO_PATTERN" is directly included [misc-include-cleaner]

      MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
                        ^


// Set size of Matrix on each processor to nlocal x nlocal
MatCreate(BoutComm::get(), &Jfd);
MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MatSetOption" is directly included [misc-include-cleaner]

      MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
      ^


// Set size of Matrix on each processor to nlocal x nlocal
MatCreate(BoutComm::get(), &Jfd);
MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "PETSC_TRUE" is directly included [misc-include-cleaner]

      MatSetOption(Jfd, MAT_KEEP_NONZERO_PATTERN, PETSC_TRUE);
                                                  ^

timestep = max_timestep;
}

// Note: Setting the SNESJacobianFn to NULL retains
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use std::min instead of > [readability-use-std-min-max]

src/solver/impls/snes/snes.cxx:6:

- #include <bout/boutcomm.hxx>
+ #include <algorithm>
+ #include <bout/boutcomm.hxx>
Suggested change
// Note: Setting the SNESJacobianFn to NULL retains
{
;
}timestep = std::min(timestep, max_timestep);

Comment thread src/solver/impls/snes/snes.cxx Outdated
/* ---------- multiplicative PID factors ---------- */
BoutReal facP = std::pow(double(target_its) / double(nl_its), kP);
BoutReal facI = std::pow(double(nl_its_prev) / double(nl_its), kI);
BoutReal facD = std::pow(double(nl_its_prev) * double(nl_its_prev) / double(nl_its)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'facD' of type 'BoutReal' (aka 'double') can be declared 'const' [misc-const-correctness]

Suggested change
BoutReal facD = std::pow(double(nl_its_prev) * double(nl_its_prev) / double(nl_its)
kI);const

Comment thread src/solver/impls/snes/snes.cxx Outdated
BoutReal fac = facP * facI * facD;
if (fac < 0.2)
fac = 0.2;
else if (fac > 5.0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

Suggested change
else if (fac > 5.0)
0.2) {
0.2;}

Comment thread src/solver/impls/snes/snes.cxx Outdated
if (fac < 0.2)
fac = 0.2;
else if (fac > 5.0)
fac = 5.0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

Suggested change
fac = 5.0;
5.0) {

src/solver/impls/snes/snes.cxx:1453:

- 5.0;
+ 5.0;
+ }

dt_new = max_timestep;
}

nl_its_prev2 = nl_its_prev;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use std::min instead of > [readability-use-std-min-max]

Suggested change
nl_its_prev2 = nl_its_prev;
fac;
ep)dt_new = std::min(dt_new, max_timestep);

Comment thread src/solver/impls/snes/snes.cxx Outdated
}

nl_its_prev2 = nl_its_prev;
nl_its_prev = static_cast<int>(nl_its);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant explicit casting to the same type 'int' as the sub-expression, remove this casting [readability-redundant-casting]

Suggested change
nl_its_prev = static_cast<int>(nl_its);
prev;

src/solver/impls/snes/snes.cxx:1463:

- ts);
+ ts;
Additional context

src/solver/impls/snes/snes.cxx:1437: source type originates from referencing this parameter

 }
                                                      ^

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

// Note: Setting the SNESJacobianFn to NULL retains
// previously set evaluation function.
//
// The SNES Jacobian is a combination of the RHS Jacobian
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use std::min instead of > [readability-use-std-min-max]

src/solver/impls/snes/snes.cxx:6:

- #include <bout/boutcomm.hxx>
+ #include <algorithm>
+ #include <bout/boutcomm.hxx>
Suggested change
// The SNES Jacobian is a combination of the RHS Jacobian
{
;
}timestep = std::min(timestep, max_timestep);

MatColoringCreate(Jfd, &coloring);
MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
MatColoringSetType(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MatColoring" is directly included [misc-include-cleaner]

oring
        ^

MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
MatColoringSetType(
coloring, MATCOLORINGGREEDY); // Parallel algorith. Better for large parallel runs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MatColoringCreate" is directly included [misc-include-cleaner]

NULL;
        ^

MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
MatColoringSetType(
coloring, MATCOLORINGGREEDY); // Parallel algorith. Better for large parallel runs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "NULL" is directly included [misc-include-cleaner]

src/solver/impls/snes/snes.cxx:1:

+ #include <cstddef>

Comment thread src/solver/impls/snes/snes.cxx Outdated
BoutReal facP = std::pow(double(target_its) / double(nl_its), kP);
BoutReal facI = std::pow(double(nl_its_prev) / double(nl_its), kI);
BoutReal facD = std::pow(double(nl_its_prev) * double(nl_its_prev) / double(nl_its)
/ double(nl_its_prev2),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'facP' of type 'BoutReal' (aka 'double') can be declared 'const' [misc-const-correctness]

Suggested change
/ double(nl_its_prev2),
-- */const

BoutReal facD = std::pow(double(nl_its_prev) * double(nl_its_prev) / double(nl_its)
/ double(nl_its_prev2),
kD);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'facD' of type 'BoutReal' (aka 'double') can be declared 'const' [misc-const-correctness]

Suggested change
kI);const

else if (fac > 5.0)
fac = 5.0;

/* ---------- update timestep and history ---------- */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

Suggested change
/* ---------- update timestep and history ---------- */
0.2) {
0.2;}

Comment thread src/solver/impls/snes/snes.cxx Outdated
fac = 5.0;

/* ---------- update timestep and history ---------- */
BoutReal dt_new = timestep * fac;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: statement should be inside braces [readability-braces-around-statements]

Suggested change
BoutReal dt_new = timestep * fac;
5.0) {

src/solver/impls/snes/snes.cxx:1453:

- 5.0;
+ 5.0;
+ }

nl_its_prev2 = nl_its_prev;
nl_its_prev = static_cast<int>(nl_its);

return dt_new;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: use std::min instead of > [readability-use-std-min-max]

Suggested change
return dt_new;
fac;
ep)dt_new = std::min(dt_new, max_timestep);

nl_its_prev = static_cast<int>(nl_its);

return dt_new;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant explicit casting to the same type 'int' as the sub-expression, remove this casting [readability-redundant-casting]

Suggested change
}
prev;

src/solver/impls/snes/snes.cxx:1463:

- ts);
+ ts;
Additional context

src/solver/impls/snes/snes.cxx:1437: source type originates from referencing this parameter

 }
                                                      ^

@malamast malamast force-pushed the snes-adaptive-pid branch from a709125 to 751a22f Compare July 24, 2025 19:34
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

timestep *= timestep_factor_on_lower_its;

timestep = std::min(timestep, max_timestep);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::min" is directly included [misc-include-cleaner]

                              ^

MatColoring coloring = NULL;
MatColoringCreate(Jfd, &coloring);
MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MatColoringCreate" is directly included [misc-include-cleaner]

NULL;
        ^

MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
MatColoringSetType(
coloring, MATCOLORINGGREEDY); // Parallel algorith. Better for large parallel runs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MATCOLORINGGREEDY" is directly included [misc-include-cleaner]

lems.
                                     ^

MatColoringSetType(coloring, MATCOLORINGSL);
// MatColoringSetType(coloring, MATCOLORINGSL); // Serial algorithm. Better for smale-to-medium size problems.
MatColoringSetType(
coloring, MATCOLORINGGREEDY); // Parallel algorith. Better for large parallel runs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "MatColoringSetType" is directly included [misc-include-cleaner]

lems.
        ^

kD);

// clamp groth factor to avoid huge changes
const BoutReal fac = std::clamp(facP * facI * facD, 0.2, 5.0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::clamp" is directly included [misc-include-cleaner]

anges
                                  ^

Copy link
Copy Markdown
Contributor

@bendudson bendudson left a comment

Choose a reason for hiding this comment

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

Thanks, @malamast !

@bendudson bendudson merged commit b8d37c2 into next Aug 12, 2025
3 checks passed
@bendudson bendudson deleted the snes-adaptive-pid branch August 12, 2025 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants