Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 163 additions & 13 deletions src/solver/impls/snes/snes.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,9 @@ SNESSolver::SNESSolver(Options* opts)
.withDefault<BoutReal>(100.)),
asinh_vars((*options)["asinh_vars"]
.doc("Apply asinh() to all variables?")
.withDefault<bool>(false)) {}
.withDefault<bool>(false)) {
has_constraints = true; ///< This solver can handle constraints
}

int SNESSolver::init() {
Solver::init();
Expand All @@ -385,6 +387,29 @@ int SNESSolver::init() {
output_info.write("\t3d fields = {:d}, 2d fields = {:d} neq={:d}, local_N={:d}\n",
n3Dvars(), n2Dvars(), neq, nlocal);

// Check if there are any constraints
have_constraints = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just give this a default value of false, unless you expect that the init() method might be called more than once.

Suggested change
have_constraints = false;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I find have_constraints is too similar a name to has_constraints (I was confused by this during my review). Could you choose another name, such as algebraic_constraints?


for (int i = 0; i < n2Dvars(); i++) {
if (f2d[i].constraint) {
have_constraints = true;
break;
}
}
for (int i = 0; i < n3Dvars(); i++) {
if (f3d[i].constraint) {
have_constraints = true;
break;
}
}

if (have_constraints) {
is_dae.reallocate(nlocal);
// Call the Solver function, which sets the array
// to one when not a constraint, zero for constraint
set_id(std::begin(is_dae));
}

// Initialise fields for storing residual of nonlinear solves
if (diagnose) {
for (const auto& f : f2d) {
Expand Down Expand Up @@ -449,6 +474,34 @@ int SNESSolver::init() {
local_residual_2d = 0.0;
global_residual = 0.0;

if (have_constraints) {
// CreatePETSc-native index sets representing the two parts of your DAE.
PetscInt istart, iend;

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: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscInt istart, iend;
PetscInt istart;
PetscInt iend;

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

    PetscInt istart, iend;
                     ^

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

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

    PetscInt istart, iend;
             ^

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

PetscCall(VecGetOwnershipRange(snes_x, &istart, &iend));
ASSERT2(iend - istart == nlocal);

std::vector<PetscInt> diff_idx;
std::vector<PetscInt> alg_idx;
diff_idx.reserve(nlocal);
alg_idx.reserve(nlocal);

for (PetscInt i = 0; i < nlocal; ++i) {
const PetscInt gi = istart + i;
if (is_dae[i] > 0.5) { // differential
diff_idx.push_back(gi);
} else { // algebraic constraint (i.e. phi)
alg_idx.push_back(gi);
}
}

PetscCall(ISCreateGeneral(BoutComm::get(), diff_idx.size(), diff_idx.data(),
PETSC_COPY_VALUES, &is_diff));
PetscCall(ISCreateGeneral(BoutComm::get(), alg_idx.size(), alg_idx.data(),
PETSC_COPY_VALUES, &is_alg));

have_is_maps = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this variable? As far as I can tell, if have_constraints is true then this will always also be true. Is there any situation when the value of have_is_maps and have_constraints could differ?

}

// Nonlinear solver interface (SNES)
output_info.write("Create SNES\n");
SNESCreate(BoutComm::get(), &snes);
Expand Down Expand Up @@ -580,6 +633,24 @@ int SNESSolver::init() {
}
}

if (have_constraints && have_is_maps && !matrix_free && pc_type == "fieldsplit") {
output_info.write("Using PCFieldSplit preconditioner for DAE system\n");

// Use PETSc fieldsplit
PetscCall(PCSetType(pc, PCFIELDSPLIT));

// Give PETSc the index sets
PetscCall(PCFieldSplitSetIS(pc, "diff", is_diff));
PetscCall(PCFieldSplitSetIS(pc, "alg", is_alg));

// Let the user configure from options (recommended)
// Example options you can set in input file:
// -pc_fieldsplit_type additive
// -fieldsplit_alg_pc_type hypre -fieldsplit_alg_pc_hypre_type boomeramg
// -fieldsplit_diff_pc_type ilu
Comment on lines +646 to +650

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be better to put these notes in documentation, rather than as a comment in the code.

//
}

// Get runtime options
lib.setOptionsFromInputFile(snes);

Expand Down Expand Up @@ -1510,26 +1581,100 @@ PetscErrorCode SNESSolver::snes_function(Vec x, Vec f, bool linear) {
switch (equation_form) {
case BoutSnesEquationForm::rearranged_backward_euler: {
// Rearranged Backward Euler
// f = (x0 - x)/Δt + f
// First calculate x - x0 to minimise floating point issues
VecWAXPY(delta_x, -1.0, x0, x); // delta_x = x - x0
VecAXPY(f, -1. / dt, delta_x); // f <- f - delta_x / dt
// F = (x0 - x)/Δt + f
// Algebraic: F = G(x) (already stored in f by rhs_function)

if (!have_constraints) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's lots of repetition in this part of the code. I've drafted an alternative approach which should simplify things, but I don't have any problems to test it on. I'll post the diff in a separate comment.


// First calculate x - x0 to minimise floating point issues
VecWAXPY(delta_x, -1.0, x0, x); // delta_x = x - x0
VecAXPY(f, -1.0 / dt, delta_x); // f <- f - delta_x / dt

} else {

ASSERT2(have_is_maps);
// Some constraints

Vec x_diff, x0_diff, delta_x_diff, f_diff;

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

s
                              ^

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

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

s
                                            ^

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

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

s
                     ^

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

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

s
             ^

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

PetscCall(VecGetSubVector(x, is_diff, &x_diff));

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: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscCall(VecGetSubVector(x, is_diff, &x_diff));
s
f;Vec x_diff;
Vec x0_diff;
Vec delta_x_diff;
Vec f_diff;

PetscCall(VecGetSubVector(x0, is_diff, &x0_diff));
PetscCall(VecGetSubVector(delta_x, is_diff, &delta_x_diff));
PetscCall(VecGetSubVector(f, is_diff, &f_diff));

PetscCall(VecWAXPY(delta_x_diff, -1.0, x0_diff,
x_diff)); // delta_x_diff = x_diff - x0_diff
PetscCall(
VecAXPY(f_diff, -1.0 / dt, delta_x_diff)); // f_diff <- f_diff - delta_x / dt

PetscCall(VecRestoreSubVector(x, is_diff, &x_diff));
PetscCall(VecRestoreSubVector(x0, is_diff, &x0_diff));
PetscCall(VecRestoreSubVector(delta_x, is_diff, &delta_x_diff));
PetscCall(VecRestoreSubVector(f, is_diff, &f_diff));
}
break;
}
case BoutSnesEquationForm::pseudo_transient: {
// Pseudo-transient timestepping. Same as Rearranged Backward Euler
// except that Δt is a vector
// f = (x0 - x)/Δt + f
VecWAXPY(delta_x, -1.0, x0, x);
VecPointwiseDivide(delta_x, delta_x, dt_vec); // delta_x /= dt
VecAXPY(f, -1., delta_x); // f <- f - delta_x
// F = (x0 - x)/Δt + f
// Algebraic: F = G(x) (already stored in f by rhs_function)

if (!have_constraints) {

VecWAXPY(delta_x, -1.0, x0, x);
VecPointwiseDivide(delta_x, delta_x, dt_vec); // delta_x /= dt
VecAXPY(f, -1.0, delta_x); // f <- f - delta_x

} else {
ASSERT2(have_is_maps);

Vec x_diff, x0_diff, delta_x_diff, f_diff, dt_vec_diff;

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

s);
                                ^

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

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

s);
                                                      ^

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

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

s);
                                              ^

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

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

s);
                       ^

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

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

s);
               ^

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

PetscCall(VecGetSubVector(x, is_diff, &x_diff));

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: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscCall(VecGetSubVector(x, is_diff, &x_diff));
s);
iff;Vec x_diff;
Vec x0_diff;
Vec delta_x_diff;
Vec f_diff;
Vec dt_vec_diff;

PetscCall(VecGetSubVector(x0, is_diff, &x0_diff));
PetscCall(VecGetSubVector(delta_x, is_diff, &delta_x_diff));
PetscCall(VecGetSubVector(f, is_diff, &f_diff));
PetscCall(VecGetSubVector(dt_vec, is_diff, &dt_vec_diff));

PetscCall(VecWAXPY(delta_x_diff, -1.0, x0_diff, x_diff));
PetscCall(
VecPointwiseDivide(delta_x_diff, delta_x_diff, dt_vec_diff)); // delta_x /= dt
PetscCall(VecAXPY(f_diff, -1.0, delta_x_diff)); // f <- f - delta_x

PetscCall(VecRestoreSubVector(delta_x, is_diff, &delta_x_diff));
PetscCall(VecRestoreSubVector(x, is_diff, &x_diff));
PetscCall(VecRestoreSubVector(x0, is_diff, &x0_diff));
PetscCall(VecRestoreSubVector(f, is_diff, &f_diff));
PetscCall(VecRestoreSubVector(dt_vec, is_diff, &dt_vec_diff));
}
break;
}
case BoutSnesEquationForm::backward_euler: {
// Backward Euler
// Set f = x - x0 - Δt*f
VecAYPX(f, -dt, x); // f <- x - Δt*f
VecAXPY(f, -1.0, x0); // f <- f - x0
// Backward Euler:
// Differential: F = x - x0 - dt*f
// Algebraic: F = G(x) (already stored in f by rhs_function)

if (!have_constraints) {

VecAYPX(f, -dt, x); // f <- x - Δt*f
VecAXPY(f, -1.0, x0); // f <- f - x0

} else {

ASSERT2(have_is_maps);
// Some constraints

Vec x_diff, x0_diff, f_diff;

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

ints
                                 ^

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

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

ints
                        ^

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

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

ints
                ^

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

PetscCall(VecGetSubVector(x, is_diff, &x_diff));

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: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
PetscCall(VecGetSubVector(x, is_diff, &x_diff));
ints
diff;Vec x_diff;
Vec x0_diff;
Vec f_diff;

PetscCall(VecGetSubVector(x0, is_diff, &x0_diff));
PetscCall(VecGetSubVector(f, is_diff, &f_diff));

PetscCall(VecAYPX(f_diff, -dt, x_diff)); // f_diff <- x_diff - dt*f_diff
PetscCall(VecAXPY(f_diff, -1.0, x0_diff)); // f_diff <- f_diff - x0_diff

PetscCall(VecRestoreSubVector(x, is_diff, &x_diff));
PetscCall(VecRestoreSubVector(x0, is_diff, &x0_diff));
PetscCall(VecRestoreSubVector(f, is_diff, &f_diff));
}
break;
}
case BoutSnesEquationForm::direct_newton: {
Expand Down Expand Up @@ -1690,6 +1835,11 @@ BoutReal SNESSolver::pid(BoutReal timestep, int nl_its, BoutReal max_dt) {
// clamp growth factor to avoid huge changes
BoutReal fac = std::clamp(facP * facI * facD, 0.2, 5.0);

// Add slow growth when convergence is good and stable
if (nl_its <= target_its && nl_its_prev <= target_its) {
fac *= 1.1; // or 1.05 for more conservative growth
}

Comment on lines +1838 to +1842

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I feel like there should be more discussion before adopting this. We are aware that timestep setting tends to be too conservative. So far we've been dealing with it by setting target_its to be much larger than it should be, as seen here. That certainly isn't a good long-term solution, but I think there were some other ideas for how this could be handled. @mikekryjak What are your thoughts?

if (pid_consider_failures && (fac > 1.0)) {
// Reduce aggressiveness if recent steps have failed often
fac = pow(fac, std::max(0.3, 1.0 - 2.0 * recent_failure_rate));
Expand Down
7 changes: 7 additions & 0 deletions src/solver/impls/snes/snes.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,13 @@ private:
int nlocal; ///< Number of variables on local processor
int neq; ///< Number of variables in total

bool have_constraints; ///< Are there any constraint variables?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
bool have_constraints; ///< Are there any constraint variables?
bool have_constraints = false; ///< Are there any constraint variables?

Array<BoutReal> is_dae; ///< If using constraints, 1 -> DAE, 0 -> AE

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 "Array" is directly included [misc-include-cleaner]

src/solver/impls/snes/snes.hxx:30:

- #include <bout/build_defines.hxx>
+ #include "bout/array.hxx"
+ #include <bout/build_defines.hxx>


IS is_diff = nullptr; // is_dae == 1
IS is_alg = nullptr; // is_dae == 0 (phi constraint and any other algebraics)
bool have_is_maps = false;

PetscLib lib; ///< Handles initialising, finalising PETSc
Vec snes_f; ///< Used by SNES to store function
Vec deriv; ///< Time derivative; only used if diagnose = true, otherwise will store in snes_f
Expand Down
Loading