Skip to content

Merge SNES constraints into refactor#3386

Open
bendudson wants to merge 6 commits into
refactor-petsc-preconfrom
merge/snes-constraints-precon
Open

Merge SNES constraints into refactor#3386
bendudson wants to merge 6 commits into
refactor-petsc-preconfrom
merge/snes-constraints-precon

Conversation

@bendudson

Copy link
Copy Markdown
Contributor

Merges the changes in #3251 into the refactor-petsc-precon branch.

malamast and others added 5 commits June 10, 2026 22:43
…on of algebraic equations.

For now it is only supported by backward_euler option. We will modify the rest of the options later.
…ork with petsc vectors instead of bout++ variables.

 I added an option to split the preconditioner so that the user can use a different PC_type for the algebraic equation.

@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


if (have_constraints) {
// Create PETSc-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;


if (have_constraints) {
// Create PETSc-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: variable 'iend' is not initialized [cppcoreguidelines-init-variables]

    PetscInt istart, iend;
                     ^

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


if (have_constraints) {
// Create PETSc-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: variable 'istart' is not initialized [cppcoreguidelines-init-variables]

    PetscInt istart, iend;
             ^

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

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

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

ints
                        ^

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

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

ints
                ^

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


Vec x_diff, x0_diff, f_diff;

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;
iVec x_diff;
Vec x0_diff;
Vec f_diff;

int neq; ///< Number of variables in total

bool have_constraints; ///< 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>

@bendudson bendudson mentioned this pull request Jun 11, 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.

Some minor suggestions

.doc("Apply asinh() to all variables?")
.withDefault<bool>(false)) {}
.withDefault<bool>(false)) {
has_constraints = true; ///< This solver can handle constraints

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.

Should this not default to false? Then L384 could be removed?

Also, maybe better to set the default in the declaration in the header

Comment on lines +384 to +397
have_constraints = false;

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

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.

We can use the shiny ranges algorithms now!

Suggested change
have_constraints = false;
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;
}
}
using std::ranges::any_of;
has_constraints = any_of(f2d, &VarStr<Field2D>::constraint)
or any_of(f3d, &VarStr<Field3D>::constraint);

I think that pointer-to-member function syntax is right, you might need a lambda:

  auto f_has_constraint = []<class T>(const VarStr<T>& f) { return f.constraint; };

as an intermediate. This has the added advantage of using all the valid bracket types in one expression :)

Comment on lines +400 to +408
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));

if (equation_form != BoutSnesEquationForm::backward_euler) {
throw BoutException(
"SNES constraints currently require equation_form=backward_euler");
}

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.

Can we do the check first?

Suggested change
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));
if (equation_form != BoutSnesEquationForm::backward_euler) {
throw BoutException(
"SNES constraints currently require equation_form=backward_euler");
}
if (equation_form != BoutSnesEquationForm::backward_euler) {
throw BoutException(
"SNES constraints currently require equation_form=backward_euler");
}
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));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants