Conversation
Scales the RHS (time derivatives) in the Jacobian calculation. Speeds up the 1D-recycling example by about x2.
Uses the magnitude of each variable to update its normalisation. Doesn't improve convergence for 1D-recycling test case (makes worse)
When using coloring, identify near-zero values and remove them from the Jacobian. This could improve efficiency by reducing the effort needed to recompute the Jacobian and preconditioner. Currently over-prunes based on initial conditions, resulting in a worse preconditioner. Need to occasionally reset and add back in the removed elements, maybe following a convergence failure.
Works well, true by default. Configures SNES so that a matrix-free approximation is used for the Jacobian-vector operator. The finite difference Jacobian is used to construct the preconditioner.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/solver/impls/snes/snes.cxx
Outdated
| // Get the the SNESSolver pointer from the function call context | ||
| SNESSolver* fctx; | ||
| err = MatFDColoringGetFunction(static_cast<MatFDColoring>(ctx), nullptr, | ||
| reinterpret_cast<void**>(&fctx)); |
There was a problem hiding this comment.
warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
reinterpret_cast<void**>(&fctx));
^There was a problem hiding this comment.
I believe this might be required because PETSc uses the wrong type (or that might be a similar but unrelated situation).
Two options:
- on line above:
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) - if we need to use this function more than once, make a wrapper with the correct type and the
NOLINTcomment inside?
|
|
||
| int SNESSolver::run() { | ||
| TRACE("SNESSolver::run()"); | ||
| int ierr; |
There was a problem hiding this comment.
warning: variable 'ierr' is not initialized [cppcoreguidelines-init-variables]
| int ierr; | |
| int ierr = 0; |
| if (loop_count % 100 == 0) { | ||
| // Rescale state (snes_x) so that all quantities are around 1 | ||
| // If quantities are near zero then RTOL is used | ||
| int istart, iend; |
There was a problem hiding this comment.
warning: variable 'istart' is not initialized [cppcoreguidelines-init-variables]
int istart, iend;
^this fix will not be applied because it overlaps with another fix
| if (loop_count % 100 == 0) { | ||
| // Rescale state (snes_x) so that all quantities are around 1 | ||
| // If quantities are near zero then RTOL is used | ||
| int istart, iend; |
There was a problem hiding this comment.
warning: variable 'iend' is not initialized [cppcoreguidelines-init-variables]
int istart, iend;
^this fix will not be applied because it overlaps with another fix
| ierr = VecGetArray(snes_x, &snes_x_data); | ||
| CHKERRQ(ierr); | ||
| PetscScalar* x1_data; | ||
| ierr = VecGetArray(x1, &x1_data); |
There was a problem hiding this comment.
warning: variable 'var_scaling_factors_data' is not initialized [cppcoreguidelines-init-variables]
| ierr = VecGetArray(x1, &x1_data); | |
| PetscScalar* var_scaling_factors_data = nullptr; |
ZedThree
left a comment
ZedThree
left a comment
There was a problem hiding this comment.
LGTM, would be good to address some of the clang-tidy comments
src/solver/impls/snes/snes.cxx
Outdated
| // Get the the SNESSolver pointer from the function call context | ||
| SNESSolver* fctx; | ||
| err = MatFDColoringGetFunction(static_cast<MatFDColoring>(ctx), nullptr, | ||
| reinterpret_cast<void**>(&fctx)); |
There was a problem hiding this comment.
I believe this might be required because PETSc uses the wrong type (or that might be a similar but unrelated situation).
Two options:
- on line above:
// NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) - if we need to use this function more than once, make a wrapper with the correct type and the
NOLINTcomment inside?
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Peter Hill <peter.hill@york.ac.uk>
| PetscScalar* snes_x_data = nullptr; | ||
| ierr = VecGetArray(snes_x, &snes_x_data); | ||
| CHKERRQ(ierr); | ||
| PetscScalar* x1_data; |
There was a problem hiding this comment.
warning: variable 'x1_data' is not initialized [cppcoreguidelines-init-variables]
| PetscScalar* x1_data; | |
| PetscScalar* x1_data = nullptr; |
| PetscScalar* x1_data; | ||
| ierr = VecGetArray(x1, &x1_data); | ||
| CHKERRQ(ierr); | ||
| PetscScalar* var_scaling_factors_data; |
There was a problem hiding this comment.
warning: variable 'var_scaling_factors_data' is not initialized [cppcoreguidelines-init-variables]
| PetscScalar* var_scaling_factors_data; | |
| PetscScalar* var_scaling_factors_data = nullptr; |
|
|
||
| if (diagnose) { | ||
| // Print maximum and minimum scaling factors | ||
| PetscReal max_scale, min_scale; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| PetscReal max_scale, min_scale; | |
| PetscReal max_scale; | |
| PetscReal min_scale; |
|
|
||
| if (diagnose) { | ||
| // Print maximum and minimum scaling factors | ||
| PetscReal max_scale, min_scale; |
There was a problem hiding this comment.
warning: variable 'max_scale' is not initialized [cppcoreguidelines-init-variables]
PetscReal max_scale, min_scale;
^this fix will not be applied because it overlaps with another fix
|
|
||
| if (diagnose) { | ||
| // Print maximum and minimum scaling factors | ||
| PetscReal max_scale, min_scale; |
There was a problem hiding this comment.
warning: variable 'min_scale' is not initialized [cppcoreguidelines-init-variables]
PetscReal max_scale, min_scale;
^this fix will not be applied because it overlaps with another fix
| const BoutReal* xdata = nullptr; | ||
| ierr = VecGetArrayRead(scaled_x, &xdata); | ||
| CHKERRQ(ierr); | ||
| load_vars(const_cast<BoutReal*>(xdata)); |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
;
^
src/solver/impls/snes/snes.cxx
Outdated
| const BoutReal* xdata = nullptr; | ||
| ierr = VecGetArrayRead(scaled_x, &xdata); | ||
| CHKERRQ(ierr); | ||
| load_vars(const_cast<BoutReal*>(xdata)); |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
;
^
src/solver/impls/snes/snes.cxx
Outdated
| const BoutReal* xdata = nullptr; | ||
| int ierr = VecGetArrayRead(x, &xdata); | ||
| CHKERRQ(ierr); | ||
| load_vars(const_cast<BoutReal*>(xdata)); |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
;
^
src/solver/impls/snes/snes.cxx
Outdated
| int rstart, rend; | ||
| MatGetOwnershipRange(B, &rstart, &rend); |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| int rstart, rend; | |
| MatGetOwnershipRange(B, &rstart, &rend); | |
| essor | |
| reint rstart; | |
| int rend; |
src/solver/impls/snes/snes.cxx
Outdated
|
|
||
| // Get index of rows owned by this processor | ||
| int rstart, rend; | ||
| MatGetOwnershipRange(B, &rstart, &rend); |
There was a problem hiding this comment.
warning: variable 'rend' is not initialized [cppcoreguidelines-init-variables]
rend;
^this fix will not be applied because it overlaps with another fix
- Making more indices const - Renaming Jacobian scaling function, removing SNES prefix to avoid potential collision with PETSc, and making function static.
| const BoutReal* xdata = nullptr; | ||
| ierr = VecGetArrayRead(scaled_x, &xdata); | ||
| CHKERRQ(ierr); | ||
| load_vars(const_cast<BoutReal*>( |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
;
^| } else { | ||
| const BoutReal* xdata = nullptr; | ||
| int ierr = VecGetArrayRead(x, &xdata); | ||
| CHKERRQ(ierr); |
There was a problem hiding this comment.
warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]
;
^
|
I've deleted a lot of the clang-tidy comments because they were not on the right lines and were very confusing |
PR #3009 included changes to allow matrix-free Jacobian-vector products, while using the finite difference Jacobian for the preconditioner. That gave significant speedups that will hopefully also help now that the coloring stencil is fixed.
2 participants
Replaces #2631
Running 1D-recycling, the first 200 output timesteps to t=1e6. BOUT++ compiled with PETSc 3.22.0, using one MPI process. Using PETSc ILU preconditioner.
Baseline:
nextbranch: 104,905 calls. 8 minutes 23 secondsImprovements (from most to least successful):
matrix_free_operatorworks so well that it is set totrueby default.