Skip to content

Simplify newton solver logic#6875

Open
gassmoeller wants to merge 4 commits intogeodynamics:mainfrom
gassmoeller:simplify_newton_logic
Open

Simplify newton solver logic#6875
gassmoeller wants to merge 4 commits intogeodynamics:mainfrom
gassmoeller:simplify_newton_logic

Conversation

@gassmoeller
Copy link
Member

I was trying to confirm that with #6860 the Newton solver works correctly for compressible models. I saw some differences (unrelated to this PR). But while debugging, I had trouble following the logic of when or when not we are assembling a Newton/defect correction system instead of a standard Stokes system. I think our logic for that is unnecessarily complicated at the moment.

In essence we need two switches:

  1. Standard or defect correction system (whether to include Newton derivatives in the defect correction system is handled by a separate scaling factor)
  2. Rebuild matrix or not.

Currently, we have three different bool variables for this, because assembling a standard matrix and assembling a newton matrix is handled by separate variables. However, we never assemble a standard and a Newton matrix, it is always one or the other.

I think the code would be easier to understand with just the two switches I described above. This PR is my attempt at this change.

The first test results I have seen look promising, but it looks like the initial Newton residual is now computed differently (it is larger with my change, therefore the method tends to converge better). I will continue looking for what is different tomorrow, but @MFraters could you check if my basic idea is right? And maybe you already spot what is different with this PR?

@gassmoeller gassmoeller force-pushed the simplify_newton_logic branch 2 times, most recently from 1c31806 to a774839 Compare February 26, 2026 13:28
@gassmoeller gassmoeller force-pushed the simplify_newton_logic branch from a774839 to af81fe8 Compare February 26, 2026 14:04
Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

@gassmoeller, see my post in #6860. To be honest, I am not entirely sure of the current state of the stabilization in the compressible case. @YiminJin, did you derive and implement that now?

Looking again at this code, it seems we made a very deliberate choice tho split them. I think it should be relatively easy to make sure you get the right results, since the test coverage (at least for hte incompressible case) should be sufficient to pick up if you don't rebuild enough. The harder thing to pick up will be to see if we now maybe rebuild too many times now.

If I remember correctly, the difference between rebuild_stokes_matrix and rebuild_newton_stokes_matrix is the rebuild_newton_stokes_matrix rebuilds the lhs of the Newton system and rebuild_stokes_matrix rebuilds the rhs. When computing the residual, you only need to rebuild the rhs, so you don't need rebuild_newton_stokes_matrix. But maybe I am misremembering this.

I think it should be easy enough to test in the old code whether at key points (for example around line 930 and 934 in newton_stokes.cc, whether rebuild_stokes_matrix == rebuild_newton_stokes_matrix in all tests with a simple assertThrow. And maybe similar for assemble_newton_stokes_matrix and assemble_newton_stokes_system. It seems you are also assuming assemble_newton_stokes_matrix == rebuild_stokes_matrix in assembly.cc now, so maybe test that as well in the old code. If that is the case than you can condense it down even more.

if (derivative_scaling_factor == 0)
{
if (scratch.rebuild_newton_stokes_matrix)
if (scratch.rebuild_stokes_matrix)
Copy link
Member

Choose a reason for hiding this comment

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

If rebuild rebuild_stokes_matrix == rebuild_newton_stokes_matrix, then you can just remove this line, since it is already checked line line 930.

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.

2 participants