Skip to content

SNES predictor refactoring - #3506

Merged
bendudson merged 13 commits into
nextfrom
snes-predictor
Oct 14, 2026
Merged

bendudson merged 13 commits into
nextfrom
snes-predictor

Conversation

@bendudson

@bendudson bendudson commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Separate the predictor into a separate Predictor class.

Set predictor type using solver:predictor:type. Options are constant, linear and be_rational that uses a damped relaxation to steady state.

An option solver:predictor:blend calculates the optimum scaling of the previous predictor step, and uses that to update the scaling of the next step.

Adds automatic detection of cases where predictors can cause numerical issues e.g. when the step size is below tolerance.

Includes documentation and unit tests.

Hermes-3 1D-threshold example

linear (default) predictor:

4.789e+03       3853       3.45e+00    89.0    0.0    0.1    1.1    9.7
9.579e+03        323       3.14e-01    81.9    0.0    0.1    8.8    9.1
1.437e+04        325       3.14e-01    82.1    0.0    0.1    8.6    9.2
1.916e+04        549       5.15e-01    84.8    0.0    0.1    5.3    9.7
2.395e+04       1335       1.20e+00    87.7    0.0    0.1    2.3    9.9
...
Run time : 19 s

be_rational predictor:

4.789e+03       1590       1.45e+00    87.5    0.0    0.1    2.5    9.8
9.579e+03        212       2.23e-01    77.1    0.0    0.1   13.1    9.7
1.437e+04        236       2.37e-01    79.0    0.0    0.1   11.7    9.1
1.916e+04        473       4.50e-01    84.2    0.0    0.1    6.2    9.4
2.395e+04        852       8.00e-01    86.2    0.0    0.1    3.5   10.1
...
Run time : 17 s

New enum `BoutSnesPredictor` can be `none` or `linear`. An error
message is printed if the previous boolean `predictor` setting is
used.

This is to enable different types of predictor to be implemented.
Separates the logic of extrapolation from previous steps
from the rest of the solver logic.

Should make experimenting with other predictor methods easier.
Small updates to documentation and error message.
@bendudson bendudson added work in progress Not ready for merging performance labels Sep 29, 2026

@github-actions github-actions Bot left a comment

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.

clang-tidy made some suggestions

Comment thread src/solver/impls/snes/snes.hxx Outdated
Comment thread src/solver/impls/snes/snes.hxx Outdated
ZedThree
ZedThree previously approved these changes Sep 30, 2026

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, some minor suggested polish though

Comment thread src/solver/impls/snes/snes.hxx Outdated
Comment thread src/solver/impls/snes/snes.hxx Outdated
Comment thread src/solver/impls/snes/snes.hxx
Use `BOUT_ENUM_CLASS_NS` to put the enums into the `bout` namespace.
@ZedThree review suggestion, using a struct rather than std::pair.

@github-actions github-actions Bot left a comment

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.

clang-tidy made some suggestions

Comment thread src/solver/impls/snes/snes.hxx Outdated
/// `x` must already be allocated and will be overwritten.
void predict(SnesPredictor predictor_type, BoutReal time, Vec& x) {
// [t0, x0] is the most recent state
const auto& [t0, x0] = this->history[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: 'x0' declared with a const-qualified typedef; results in the type being '_p_Vec *const' instead of 'const _p_Vec *' [misc-misplaced-const]

    const auto& [t0, x0] = this->history[0];
                     ^
Additional context

/usr/lib/petscdir/petsc3.22/x86_64-linux-gnu-real/include/petscvec.h:25: typedef declared here

typedef struct _p_Vec *Vec;
                       ^

Comment thread src/solver/impls/snes/snes.hxx Outdated
BOUT_DO_PETSC(VecCopy(x0, x));
return;
case SnesPredictor::linear: {
const auto& [t1, x1] = this->history[1];

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: 'x1' declared with a const-qualified typedef; results in the type being '_p_Vec *const' instead of 'const _p_Vec *' [misc-misplaced-const]

      const auto& [t1, x1] = this->history[1];
                       ^
Additional context

/usr/lib/petscdir/petsc3.22/x86_64-linux-gnu-real/include/petscvec.h:25: typedef declared here

typedef struct _p_Vec *Vec;
                       ^

Symbols moved into `bout` namespace
- be_rational uses three states to work out a backward Euler damping
  factor and extrapolate.

- Quality measure that evaluates how good predictors are.
Scales the predicted step size by a factor that is adapted.
After each step the optimal factor is calculated and used
to update the factor for the next step.
Moves predictor options into subsection of solver options.

e.g. Rather than `solver:predictor_blend` it's `solver:predictor:blend`
github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

github-actions[bot]

This comment was marked as outdated.

Fixed call to VecAXPY with incorrect argument order.

Additional unit testing.

Documentation and code docstrings consistent with implementation.

@github-actions github-actions Bot left a comment

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.

clang-tidy made some suggestions

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

// Predicted state
// `x` always lies between `x0` (constant prediction) and the linear prediction.
x_data[i] = x0_data[i] + dt * s0 * damp;

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: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]

Suggested change
x_data[i] = x0_data[i] + dt * s0 * damp;
rediction.(

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

- mp;
+ mp);

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

BoutReal Predictor::quality(Vec x0, Vec x_new, Vec x_pred) {
PetscReal norm_num, norm_den;

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 'norm_num' is not initialized [cppcoreguidelines-init-variables]

, Vec x_pred) {
                            ^

this fix will not be applied because it overlaps with another fix

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

BoutReal Predictor::optimal_weight(Vec x0, Vec x_new, Vec x_pred) {
PetscReal norm_delta, norm_align;

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 'norm_delta' is not initialized [cppcoreguidelines-init-variables]

, Vec x_pred) {
                            ^

this fix will not be applied because it overlaps with another fix

Comment thread src/solver/impls/snes/snes.cxx
Comment thread src/solver/impls/snes/snes.cxx
Comment thread src/solver/impls/snes/snes.hxx
Comment thread src/solver/impls/snes/snes.hxx
@bendudson bendudson removed the work in progress Not ready for merging label Oct 13, 2026
Makes printing of the `quality` metric optional.

Renamed `quality` to `prediction_error_ratio` because it's a metric
where smaller is better; minimising quality doesn't seem right.
Only prints blending diagnostics when `diagnose=true`

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Just wondering if the use of this-> is a deliberate choice, or just an LLMism?

@bendudson

Copy link
Copy Markdown
Contributor Author

Thsnks @ZedThree ! The this-> is an @oparry-ukaea 'ism. I quite like it so used it here.

@bendudson
bendudson merged commit 603debf into next Oct 14, 2026
23 checks passed
@bendudson
bendudson deleted the snes-predictor branch October 14, 2026 16:08
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.

2 participants