Skip to content

[Fix]: NewtonRaphsonSolver convergence check and error handling#1285

Merged
JordiManyer merged 4 commits into
gridap:masterfrom
Ady0333:fix/newton-convergence-check
Apr 27, 2026
Merged

[Fix]: NewtonRaphsonSolver convergence check and error handling#1285
JordiManyer merged 4 commits into
gridap:masterfrom
Ady0333:fix/newton-convergence-check

Conversation

@Ady0333
Copy link
Copy Markdown
Contributor

@Ady0333 Ady0333 commented Apr 24, 2026

Summary

Fixes #1284

Two related fixes for NewtonRaphsonSolver:

  1. Initial convergence check - Actually test if starting residual is below tolerance
  2. Error handling - Replace @unreachable with informative error message

Changes

In src/Algebra/NLSolvers.jl:

Line 81 - Initial check:

# Before: return (false, m0)
# After:  return (m0 < nls.tol, m0)

Line 67 - Non-convergence handling:

# Before: @unreachable
# After:  error("NewtonRaphsonSolver failed to converge in $(nls.max_nliters) iterations. Final residual: $m")

Behavior

Before:

  • Starting from converged state → crash with "@unreachable"
  • Relative tolerance m < tol*m0 unsatisfiable when m0 ≈ 0

After:

  • Already-converged initial state → exits immediately
  • Non-convergence → informative error with diagnostics

Testing

Tested with Newton starting at exact solution (zero residual):

  • Previously: crashed with "@unreachable"
  • Now: exits cleanly on first convergence check

All existing Gridap tests pass.


Related

Related to #1282 - part of broader Newton solver convergence testing

Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
@Ady0333
Copy link
Copy Markdown
Contributor Author

Ady0333 commented Apr 24, 2026

Hello @JordiManyer , @Antoinemarteau and @oriolcg !!! Please review this pr and let me know if any changes are needed...

~Thanks

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.82%. Comparing base (670f802) to head (a6cb5dd).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/Algebra/NLSolvers.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1285   +/-   ##
=======================================
  Coverage   88.82%   88.82%           
=======================================
  Files         227      227           
  Lines       29753    29753           
=======================================
  Hits        26429    26429           
  Misses       3324     3324           
Flag Coverage Δ
drivers 39.35% <0.00%> (ø)
extensions 5.13% <0.00%> (ø)
unit-adaptivity 40.11% <0.00%> (ø)
unit-basics 14.67% <50.00%> (ø)
unit-celldata 21.15% <0.00%> (ø)
unit-fespaces-1 32.34% <0.00%> (ø)
unit-fespaces-2 38.83% <0.00%> (ø)
unit-fields 17.59% <0.00%> (ø)
unit-geometry 28.80% <0.00%> (ø)
unit-multifield 30.86% <0.00%> (-0.12%) ⬇️
unit-odes 28.75% <0.00%> (ø)
unit-referencefes 34.28% <0.00%> (ø)
unit-visualization 11.92% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JordiManyer
Copy link
Copy Markdown
Member

Thanks @Ady0333 for the contribution. Changes look good, please update NEWS.md so that I can merge. This PR could have been bundled with the other one, it's addressing the same kind of things.

Also, kindly stop tagging everyone when you open a PR. People have their own lives and our inboxes are already quite full. When you leave a PR, someone will eventually look into it. Unless your PR has been sitting without response for over a week, it is really not necessary.

Signed-off-by: Ady0333 <adityashinde1525@gmail.com>
@Ady0333
Copy link
Copy Markdown
Contributor Author

Ady0333 commented Apr 27, 2026

Thanks @Ady0333 for the contribution. Changes look good, please update NEWS.md so that I can merge. This PR could have been bundled with the other one, it's addressing the same kind of things.

Also, kindly stop tagging everyone when you open a PR. People have their own lives and our inboxes are already quite full. When you leave a PR, someone will eventually look into it. Unless your PR has been sitting without response for over a week, it is really not necessary.

Thanks for the review. I have updated the NEWS.md and will make sure not to tag anyone...apologies for that!!!

@JordiManyer JordiManyer merged commit 0a138dd into gridap:master Apr 27, 2026
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.

[bug]: NewtonRaphsonSolver crashes with @unreachable when starting from converged solution

2 participants