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.

Sign up for GitHub

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

New NewtonSolver._residual0 initialisation (residual) #3611

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

New NewtonSolver._residual0 initialisation (residual) #3611

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b67d74e
New NewtonSolver._residual0 initialisation (residual)
SalzmanA Jan 30, 2025
4392aa8
Merge branch 'main' into newton_residual_init
jorgensd Jan 31, 2025
c83d891
Fix indentation errors
SalzmanA Feb 16, 2025
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
13 changes: 6 additions & 7 deletions cpp/dolfinx/nls/NewtonSolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,10 @@ std::pair<int, bool> nls::petsc::NewtonSolver::solve(Vec x)
// Check convergence
bool newton_converged = false;
if (convergence_criterion == "residual")
{
std::tie(_residual, newton_converged) = this->_converged(*this, _b);
_residual0 = _residual;
}
else if (convergence_criterion == "incremental")
{
// We need to do at least one Newton step with the ||dx||-stopping
Expand Down Expand Up @@ -215,13 +218,6 @@ std::pair<int, bool> nls::petsc::NewtonSolver::solve(Vec x)
if (_system)
_system(x);
_fnF(x, _b);
// Initialize _residual0
if (_iteration == 1)
{
PetscReal _r = 0.0;
VecNorm(_dx, NORM_2, &_r);
_residual0 = _r;
}

// Test for convergence
if (convergence_criterion == "residual")
Expand All @@ -232,6 +228,9 @@ std::pair<int, bool> nls::petsc::NewtonSolver::solve(Vec x)
// set.
if (_iteration == 1)
{
PetscReal _r = 0.0;
Copy link
Member

Choose a reason for hiding this comment

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

Over indented?

VecNorm(_dx, NORM_2, &_r);
_residual0 = _r;
_residual = 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the PR, but I think "residual" initialization _residual = 1.0 for the incremental criterion should happen above, outside of the loop just as it does for residual criterion. In the current code, even if the first ||dx|| < atol I would have to wait one additional iteration.

Besides the naming is not great and what we call "residual" for incremental criterion is actually increment norm...

Copy link
Member

Choose a reason for hiding this comment

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

I think this piece of code could be completely refactored so that the convergence criterion can be switched out based on passing a different std::function?

Copy link
Member

Choose a reason for hiding this comment

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

It can? You can do set_convergence_check?
https://github.com/search?q=repo%3AFEniCS%2Fdolfinx%20set_convergence_check&type=code
If you mean that we should have a function that includes the modification of the tolerances, I'm not sure that is worth-while.

newton_converged = false;
}
Expand Down