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

recompute pressure scaling for every Stokes solve #5117

Merged
merged 5 commits into from Mar 29, 2023

Conversation

gassmoeller
Copy link
Member

Currently the pressure scaling factor is only computed once per timestep. We have seen in a model that this can lead to convergence problems, if the viscosity depends strongly on the solution and our initial guess for viscosity is bad. Recomputing for every nonlinear iteration makes more sense (it is not very expensive and on average improves convergence of the linear solver).
This is more accurate for two reasons:

  1. In later nonlinear iterations viscosity can be very different.
  2. Even in models without nonlinear iterations, the Stokes system is assembled with a different viscosity than the one currently used for the pressure scaling. That is, because temperature and composition are already solved and may influence the viscosity.

I have moved the computation into the assembly of the Stokes system. There was a comment claiming that this is not possible, because constraints may depend on the pressure scaling, but after looking through the code I think this comment was outdated. The pressure scaling does not seem to be used for any constraints (it is only used in assembly functions, even for the melt solver).

Please do not merge this yet, this change will affect a lot of tests and I want to take a look at the changes first.

@tjhei
Copy link
Member

tjhei commented Mar 26, 2023

There was a comment claiming that this is not possible, because constraints may depend on the pressure scaling

The only thing I can think of are the melt pressure constraints. Everything else should not depend on the pressure scaling.

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

I did not check the changes in test results, but the code change is okay.

@gassmoeller
Copy link
Member Author

I went through the test changes and am reasonably sure they all make sense. I have to correct myself slightly in that the number of iterations does not always go down, in some models it increases. However, in any case computing the pressure scaling right before solving the Stokes system seems more consistent and accurate.

The only thing I can think of are the melt pressure constraints. Everything else should not depend on the pressure scaling.

I checked the melt pressure constraints. We do create melt pressure constraints before assemble_stokes, but we do not use the pressure scaling for them. The melt pressure constraints are all set to 0 in non-melt cells, independent of pressure scaling. Anyway, with the new code the pressure_scaling should be signaling_nan any time before the first Stokes assembly, and it does not trigger any floating point exceptions, so I think we are safe.

The last failing tester should be fixed by tomorrow.

@tjhei
Copy link
Member

tjhei commented Mar 29, 2023

I have to correct myself slightly in that the number of iterations does not always go down, in some models it increases

Not too surprising, as a wrong scaling can make solving easier or harder.

@tjhei tjhei merged commit 3ce4141 into geodynamics:main Mar 29, 2023
6 of 7 checks passed
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.

None yet

2 participants