-
Notifications
You must be signed in to change notification settings - Fork 235
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
Fixup newton solver and elastic rheology #5580
Fixup newton solver and elastic rheology #5580
Conversation
…tic_strain_rate in ElasticOutputs
…ity::calculate_viscoelastic_strain_rate()
…e contribution of elastic shear when elasticity is enabled
@YiminJin - Thanks for the contribution and I'm excited to see how this improves the convergence behavior in this class of models. Can you briefly summarize here (or post an image) of how the convergence behavior varied when these changes were applied? Question - would it be possible to try applying these updates on top of the revised VEP formulation effort (#4370) @anne-glerum has been leading? Perhaps easiest to discuss live at an ASPECT user (or other) meeting? We are still struggling with periodic poor nonlinear convergence behavior in some of the most complicated setups when using #4370, and perhaps your proposed changes will fix this. Thank you again for the contribution and looking forward to discussing further! |
@naliboff Thanks for your comment. I have plotted a diagram to compare the convergence rates of DCP method and Newton method (with old and new SPD factor) in the first 10 time steps of the gerya_2019 benchmark:
The modifications above both change the linear system (the elastic force affects the first nonlinear iteration step for defect-correction methods), hence there are significant differences between the results obtained by this branch and the main branch . Nevertheless, the modifications are good for convergence -- the relative residual of DCP method (in 100 nonlinear iterations) is decreased by one magnitude comparing to the main branch. Question: I have eyeballed #4370, but there are so many modifications that I cannot get the key points. Could you please tell me what the modifications are mainly for? And what is the "correct stresses" in the title? |
@YiminJin - Thanks for providing the plots of convergence behavior and this is really promising! In short, the stresses in the current VE implementation diverge very slightly from simple analytical solutions, and the problem becomes more pronounced for VEP problems with strong strain localization. #4370 is a fix for these issues, which ended up being a major overhaul. I think it would be best to meet with the group working on the VEP revision live, and then we can decide how best to proceed with this PR. I had a quick look at this PR, and early next week will try to see how easy it would be to implement your changes into #4370 (or whether they should be merged first). Cheers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YiminJin - I've done an initial review, an overall the code is in great shape. The modifications to use the effective strain rate make sense, but others will need to comment on the SPD modifications.
I ran a test comparing your branch with the current main branch based on a modified version of the Kaus brick benchmark I have been using to test #4370. That PRM and the statics files are attached (ran out of time before I could make plots).
A quick observations before diving into the convergence behavior later on:
Running with your PR took about three times as long relative to using the current main branch. Have you observed something similar in your tests, or was both the convergence behavior and solver behavior improved with these proposed changes? I wonder if it is related to the Newton parameters I selected, as previously I was just using DCP.
Looking forward to discussing further!
this-pr_test1_log.txt
main_test1_log.txt
[main_test1_log.txt]
d1e20-par-cav-test1.prm.txt
// Fill elastic force outputs (See equation 30 in Moresi et al., 2003, J. Comp. Phys.) | ||
force_out->elastic_force[i] = -1. * ( average_viscoelastic_viscosity / calculate_elastic_viscosity(average_elastic_shear_moduli[i]) * stress_old ); | ||
|
||
// The viscoelastic strain rate is needed only when the Newton method or the DCP method is used for solving. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The viscoelastic strain rate is needed only when the Newton method or the DCP method is used for solving. | |
// The viscoelastic strain rate is needed only when the Newton or defect correction picard nonlinear solvers are selected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naliboff Thanks very much for reviewing my codes!
I have tried the benchmark and the modified Newton solver performs poorly :-(. I think the problem is caused by the following two facts:
- In the first 4 time steps the solver should converge within 2 iterations since plastic yielding has not occurred and the system is linear. However, the modified Newton solver needs a lot more iterations to converge. This is because on the RHS of the Newton system I have changed
$$2\eta\dot{\boldsymbol{\varepsilon}} - \frac{\eta}{G\Delta t}\boldsymbol{\tau}^{\text{old}}$$
to
$$2\eta\dot{\boldsymbol{\varepsilon}}_{ve},$$
which looks identical. However, the viscoelastic strain rate is calculated by
where deviator() is applied to the current strain rate (this is necessary, since the viscosity is calculated with the deviatoric strain rate). Consequently, the RHS is not precisely equal to the system residual. This problem has been fixed by restoring the original RHS, and the covergence rate is much better (see the log files below), but still not as fast as expected.
- The GMG solver has been selected in this benchmark, which requires the viscosity to be averaged in each cell. However, function MaterialModel::MaterialAveraging::average() is called AFTER MaterialModel::evaluate(), which means that
$\eta$ is changed after$\partial\eta/\partial\dot{\boldsymbol{\varepsilon}}$ is calculated, hence the LHS can no longer represent the system tangent. Therefore, the modified Newton solver cannot be used simultaneously with GMG solver for now. To fix this problem we may have to do a lot more work (to calculate$\partial\eta/\partial\dot{\boldsymbol{\varepsilon}}$ after cell averaging).
The following log files are the results of the main branch, this PR (after fixing problem 1), and without cell averaging. The convergence rate without cell averaging is much faster than the previous two.
log_main.txt
log_pr5580.txt
log_no_avg.txt
|
||
// The viscoelastic strain rate is needed only when the Newton method or the DCP method is used for solving. | ||
elastic_out->elastic_force[i] = -out.viscosities[i] / calculate_elastic_viscosity(average_elastic_shear_moduli[i]) * stress_old; | ||
if (Parameters<dim>::is_defect_correction(this->get_parameters().nonlinear_solver)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this condition also capture the case for the full Newton solver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. In fact, after restoring the RHS only the full Newton solver needs the viscoelastic strain rate now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does this condition need to be updated to only reflect the full Newton solver?
Thanks for your work on this and making this pull request @YiminJin! I looked over the changes, and they look reasonable to me, but I am not very familiar with the elasticity code and I will need a bit more time to think about the spd factor. Scanning over the tester result your fixes do seem to generally improve the convergence, which is great. Can you indent your code and add to commit which updates the tester results in the pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for these changes, it's wonderful to see a speed up in convergence! I've added a few small comments; keeping the existing function in the elasticity plugin would be great. I leave the SPD factor changes to @MFraters :)
|
||
// The viscoelastic strain rate is needed only when the Newton method or the DCP method is used for solving. | ||
elastic_out->elastic_force[i] = -out.viscosities[i] / calculate_elastic_viscosity(average_elastic_shear_moduli[i]) * stress_old; | ||
if (Parameters<dim>::is_defect_correction(this->get_parameters().nonlinear_solver)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does this condition need to be updated to only reflect the full Newton solver?
@@ -519,7 +552,7 @@ namespace aspect | |||
|
|||
Assert(!this->get_parameters().enable_elasticity | |||
|| | |||
outputs.template get_additional_output<MaterialModel::ElasticOutputs<dim>>()->elastic_force.size() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still needs to be true, right? The elastic_force is used in line 370 (new version)
Co-authored-by: anne-glerum <[email protected]>
@MFraters Thanks for checking the modifications :-) But I don't understand the meaning of "add to commit which updates the tester results" and how to indent the code. I have tried "clang-format -i ", but it changes a lot of things (including the original codes). |
Co-authored-by: anne-glerum <[email protected]>
when you run the tests locally, you will notice that a lot of test fail. You can also see that with the github testers, which are failing. That is correct in this case, because you changes changed the results of the tester (different ammount of iterations, different residuals, ets.) Here is how you can update the test results: https://aspect-documentation.readthedocs.io/en/latest/user/extending/testing/writing-tests.html. Once you applied the diff, you can make a commit out of that and push it.
aspect uses astyle 2.04 (that is a very specific version of astyle, not the latest) for indenting, not clang-format. You will need to install it (see https://github.com/geodynamics/aspect/wiki/Indenting-code-and-installing-the-correct-version-of-astyle for more info). Once you have done that you can run I hope this helps! |
@MFraters Thank you for the detailed illustration. I haven't started to deal with the testers an the formats though, because I'm trying to fix the problem with material averaging. Newton method with material averaging is far more complex than I expected. I made a straightforward derivation and updated the codes accordingly. The derivation is in the following pdf:
kaus_2010.txt |
I have fixed the Stokes newton assembler with material averaging. Now with block AMG solver and harmonic average, both the kaus_2010 benchmark and the gerya_2019 benchmark can converge within 10 nonlinear iterations in each time step. Please test the newest version of this PR @naliboff . It is suggestive to switch to Newton method earlier in order to get faster convergence, like the prm files below. To make the Newton solver achieve optimal convergence with material averaging, 3 modifications are required:
Without any of the 3 modifications, the Newton method would show poor performance. I have also made the 3 modifications in source/simulator/stokes_matrix_free.cc, mainly in function local_apply(). However, the Newton method converges even slower than before. This time I cannot hope to figure it out by myself, since I have little knowledge about the matrix-free method. Please check the modifications in source/simulator/stokes_matrix_free.cc and see if it really does the assembling as Eqs. (18), (20) and (22) in the pdf file @MFraters @gassmoeller @tjhei . If the problem cannot be solved, I will restore the original file and deal with the format and tester issues to make this PR ready for merge. |
Eureka! Problem solved! It's lucky that I took a final glance at local_apply() and noticed that I misunderstood the functions get_value() and get_gradient(). Now the Newton method works for both AMG solver and GMG solver, and the convergence rate is much better than before. I have pushed the new commits with the indentation problem settled by astyle-2.04. However, I have entered the link labelled "Details" but did not find the "changes-test-results.diff" file. Could you please tell me how to fetch this file? @MFraters @anne-glerum |
great, thank you. I will take a closer look...
Identation is correct now, so don't worry. |
} | ||
else | ||
velocity.submit_symmetric_gradient(sym_grad_u, q); | ||
|
||
pressure.submit_value(-cell_data->pressure_scaling * div, q); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you move this to the end from above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have followed the conventional sequence velocity -> pressure when rewriting this part... It is better to preserve the original sequence since the terms to be submitted to pressure has already been done far above. However, the naming of variables in the original code is a little bit confusing, so I rewrite it again to make it more readable.
It is worth noting that my modification is simple but not efficient. The function local_apply() is called in each and every vector multiplication of the CG solver, so it is inefficient to take another loop over the quadrature points in this function. A better way is to store the averages of newton derivatives in cell_data, which is only calculated once in each nonlinear iteration. But I do not know how to implement this idea yet.
@YiminJin - This is fantastic news!! I am going to give this a try with a version of the continental extension cookbook modified to included elasticity and other adjustments, which has not been able to converge past a certain stage if the plastic damper viscosity is sufficiently low.
Here is you would do this for the 9.5-v3 tester:
|
@naliboff Thanks a lot! The changes in test results have been applied now. By the way, when using material averaging with the Newton method it is recommended to use "SPD" instead of "PD", because the material averaging makes the top-left block lose its symmetry. |
I don't understand why the tester still fails when changes-test-results-9.5.diff and changes-test-results-master.diff are vacant. Is it possible that some compile error occurred? |
Some tests are crashing with an error message. You can go to "details" under dealii-9.5 tester:
It seems that the assemble_advection_system() needs to supply evaluation points now. |
@YiminJin - I ran a modified version of the continental extension cookbook (elasticity, particles, other changes) with AMG + Newton solver with this branch and the main branch. The results are consistent with your findings above, in that the updated scheme typically produces convergence (to a tolerance of 1e-5) in ~ 10 nonlinear iterations, whereas running the same problem with the current main branch or #4370 will typically not produce convergence within the maximum allotted (200) number of nonlinear iterations. The prm file and a plot of the number of nonlinear iterations required per time step is attached, but note that the results from the model using the main branch are still running. There is one time step using this branch where it reaches the maximum number of nonlinear iterations, and an examination of that time step revealed the nonlinear residual cycling around a tolerance near ~ 2e-5 - 5e-5. So, one anomaly, but overall this is a dramatic improvement in convergence behavior for models that run for a much longer period of time! |
@tjhei Thanks for the instruction. I have found the problem.
This is because when doing material averaging in filling elastic outputs and reaction terms (source/material_model/rheology/elasticity.cc), I have assumed the quadrature formula for evaluating material properties is Introspection::quadratures, but it is not always the case (for instance, when computing the artificial viscosity the quadrature becomes QTrapezoid). I have not found an easy way to solve it, so I just excluded the "project to Q1 (only viscosity)" scheme from consideration. I have added a "TODO" to mark the problem. |
@naliboff Great to hear that :-) Thanks for carrying out the experiments and plotting the comparison chart. About the one time step that reaches the upper limit of nonlinear iterations, I think the problem lies in the constitutive model. Technically, V(E)P problems with friction angle greater than dilatancy angle are all ill-posed, because the plastic flow is non-associated. My modifications only improve the convergence rate, but do nothing to the well-posedness of the problem. For that, we still need the regularization methods. If a viscous damper with constant viscosity makes the shear bands too blurred, maybe a power-law viscoplastic regularization model (10.1029/2021GC009675) helps. |
If someone would like to modify this PR, please notice that the part related with matrix-free method in my derivation is wrong. Just ignore it. |
/rebuild |
Could someone summarize the current state of this PR? I see that Timo approved it at an earlier point, but it looks like there were a few changes afterwards. Tests are passing, so is this PR in principle ready for a final look and/or merge? |
@gassmoeller Timo approved this PR last week and pointed out that there might be some unnecessary changes in function local_apply(), so I rewrote that part, and then made some more changes to deal with the indentation and the testers. This PR has some shortcomings:
All that aside, the purpose of this PR has been completed. It greatly improves the convergence behavior of the Newton method according to John's and my experiments. If the shortcomings are currently acceptable, then I think it is ready for merge. |
I would want to take another look, but I am traveling right now. Secondly, I will have one of my students test the performance implications. But overall, I am happy to see this merged. Thank you, again! |
I'll have another look tomorrow, hope that's okay! |
Absolutely, thanks for all the feedback. I just wanted to check in to get an overview and see what / if anything still needs to be done here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the updates @YiminJin! From my side, there are only a few small questions/edits left :)
@@ -239,36 +239,93 @@ namespace aspect | |||
} | |||
|
|||
|
|||
namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can update the comment and check in line 209 and onwards (elasticity.cc) now that the material averaging of the viscosity is included in the RHS term and the reaction_outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about updating the lines starting from line 209?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be OK if we remove the assertion from line 213 to 224 and add an assertion that prohibits the usage of Q1 projection average. But still, when using particle-in-cell method the reaction terms for elasticity cannot be averaged, so we may have to notify the users about this somewhere.
correct spelling Co-authored-by: anne-glerum <[email protected]>
Co-authored-by: anne-glerum <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanations! There is still the comment to be updated, but I'm fine with merging.
@@ -239,36 +239,93 @@ namespace aspect | |||
} | |||
|
|||
|
|||
namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about updating the lines starting from line 209?
Looks good. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this! I think the results speak for themselves. I also did a small amount of testing on my models and there seemed so be generally a small improvement.
Based on this I am happy to see this merged.
This PR is a simple solution for issue #5555. The purpose is to fixup some problems related with the Newton solver.
The major modifications are listed as follows:
Modify the SPD factor. This modification is based on the fact that in ALL the material models in ASPECT, the constitutive relation is isotropic and the final viscosity can be expressed as a function of the second invariant of$\dot{\boldsymbol{\varepsilon}}$ . Define $E \equiv \dfrac{\partial\eta}{\partial\dot{\boldsymbol{\varepsilon}}}:\dot{\boldsymbol{\varepsilon}}$ . When $E < -c_{\text{safety}}\eta$ , the new SPD factor is expressed as
$$\alpha =c_{\text{safety}}\left|\frac{\eta}{E}\right|.$$ $E$ can be further simplified to
$$E = \frac{\partial\eta}{\partial\dot{\varepsilon}}\dot{\varepsilon},$$ $\dot{\varepsilon}$ is the second invariant of $\dot{\boldsymbol{\varepsilon}}$ . Using this expression, we don't have to calculate the derivative of $\eta$ with respect to each component of the strain rate tensor. Here I preserve the original expression in case that new material models with viscosity unable to be expressed as function of $\dot{\varepsilon}$ are introduced to ASPECT in the future.
In fact, the expression of
where
Add viscoelastic_strain_rate in ElasticOutputs and modify some member functions in class rheology::Elasticity. This is because we need$\dot{\boldsymbol{\varepsilon}}^{\text{ve}}$ when the Newton method is applied.
apply deviator() to the strain rate in system jacobian and include the contribution of elastic shear when elasticity is enabled. The reason has been illustrated in issue Compute a better alpha factor in the implementation of the SPD-ness of the Newton method. #5555.
I have tested the modified codes with the gerya_2019 benchmark. The prm files for VP and VEP models with Newton method are
gerya_2019_vp.txt
and
gerya_2019_vep.txt
respectively. Although the results look correct, it is still possible that there are mistakes in my modifications. Please let me know if anyone finds problems.
Before your first pull request:
For all pull requests:
For new features/models or changes of existing features: