Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CVODE constraints and max_noinlinear_iterations options #2303

Merged
merged 5 commits into from
May 7, 2021

Conversation

johnomotani
Copy link
Contributor

Interface to two more options provided by CVODE solver:

  • solver:apply_positivity_constraints - if set to true, read an option positivity_constraint in each variable subsection, which can be set to none (the default), positive, non_negative, negative, or non_positive. If any of these constraints is violated at the end of an internal timestep, CVODE will go back to the previous step and continue with a shorter timestep.
  • solver:max_nonlinear_iterations - allow changing the maximum number of nonlinear iterations allowed for each timestep. CVODE default is 3. Might be useful to increase if the number of linear iterations per Newton iteration is small, but a very brief test did not observe any benefit.

@johnomotani johnomotani added backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 feature labels May 5, 2021
Uses CVodeSetConstraints function to allow constraints to be applied to
variables to be: none (default, no constraint), positive, non_negative,
negative, or non_positive.
Copy link
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

src/solver/impls/cvode/cvode.cxx Show resolved Hide resolved
// Loop over 2D variables
for (std::vector<BoutReal>::size_type i = 0; i < f2dtols.size(); i++) {
if (bndry && !f2d[i].evolve_bndry) {
continue;
}
abstolvec_data[p] = f2dtols[i];
option_data[p] = f2dtols[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

option_data[p] = f2dtols[i];
^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't understand this error - I don't think there's any pointer arithmetic here, just accessing a C-array with an int index...

Copy link
Member

Choose a reason for hiding this comment

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

The C-array indexing is basically sugar for pointer arithmetic: the compiler turns it into *(option_data + p) = f2dtols[i] (and this is why array[0] == 0[array] works even though it looks like bad magic). This is a bit more obvious looking at the function signature, which takes a BoutReal* rather than an array type.

There isn't a great deal we can do to do this properly, given that we don't own N_Vector (and also that it's pretty much an opaque pointer). In C++20, we could take a std::span here instead of BoutReal*.

I think we could use N_VMake_Parallel to create abstolvec with our own data (e.g. Array), and then pass that directly to set_abstol_values.

(Incidentally, I'm pretty sure we could do something like that to avoid copying the derivatives in and out of the solver's data structures if we could somehow allocate a big block of consecutive memory for the derivatives in the first place)

@@ -686,7 +776,7 @@ void CvodeSolver::loop_abstol_values_op(Ind2D UNUSED(i2d), BoutReal* abstolvec_d
if (bndry && !f3d[i].evolve_bndry) {
continue;
}
abstolvec_data[p] = f3dtols[i];
option_data[p] = f3dtols[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use pointer arithmetic [cppcoreguidelines-pro-bounds-pointer-arithmetic]

option_data[p] = f3dtols[i];
^

src/solver/impls/cvode/cvode.hxx Outdated Show resolved Hide resolved
src/solver/impls/cvode/cvode.hxx Outdated Show resolved Hide resolved
ZedThree
ZedThree previously approved these changes May 6, 2021
Copy link
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.

LGTM, maybe a couple of things to polish

@ZedThree ZedThree merged commit 5357d67 into next May 7, 2021
@ZedThree ZedThree deleted the sundials-more-options-next branch May 7, 2021 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants