-
Notifications
You must be signed in to change notification settings - Fork 251
Implementation of plastic dilation #6373
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
Conversation
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 don't understand the reasoning for splitting dilation into LHS and RHS and the change to the -2/3 term for incompressible simulations, but the rest of the code looks clean and correct!
@naliboff can you take a look and then we can maybe discuss together?
|
||
// Only assemble this term if we are running incompressible, otherwise this term | ||
// is already included on the LHS of the equation. | ||
if (enable_prescribed_dilation && !material_model_is_compressible) |
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 is this no longer needed?
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 boolean material_model_is_compressible
determines whether the assembler StokesCompressibleStrainRateViscosityTerm
is executed. I have forced the assembler to be executed when plastic dilation is enabled (see line 76 in source/simulator/assembly.cc
and line 55 in source/simulator/newton.cc
), so the volumetric part of strain rate is subtracted from the LHS, and this term is no longer needed.
if (cell_data->is_compressible || | ||
cell_data->enable_prescribed_dilation) | ||
for (unsigned int d=0; d<dim; ++d) | ||
velocity_terms[d][d] -= viscosity_x_2 / 3. * div_u; |
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 did not see this change in the matrix-based version. Is this there as well?
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, the same change is made in the matrix-based assembler (line 76 in source/simulator/assembly.cc
and line 55 in source/simulator/newton.cc
).
this->get_newton_handler().parameters.newton_derivative_scaling_factor; | ||
|
||
active_cell_data.enable_newton_derivatives = true; | ||
active_cell_data.enable_newton_derivatives = (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.
With your change we now create the table for newtion_derivatives if dilation=true but we are not using a Newton solver. Is this necessary?
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 the Newton derivative tables are not created if the condition on the RHS of line 433 is not fulfilled. I have added if (active_cell_data.enable_newton_derivatives)
before reinitializing those tables (line 461).
Finally, it would be good to have some simple tests that exercise the new code. In fact the tests are currently broken:
|
@YiminJin - Thank you for continuing to work on this and open the PR! A few initial comments and thoughts from your initial post (more to come in the review):
|
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 - Nice work on this PR, it is in very good shape in terms of the structure, readability, and formatting. I confess I am having a very hard time evaluating the correctness of specific details, without seeing a higher-level summary of the equations and approach. Maybe we can discuss this in person? In any case, thank you again for pushing this forward and this looks like great progress!
I think I misunderstood the theory about associated/non-associated plastic flow. I reviewed the theory and it did not say that associated plastic flow is essentially mesh-independent. On the contrary, since we use a local method for plastic yielding (not global methods such as the strain gradient method), the thickness of the shear bands should be mesh-dependent. I will try to figure out why the shear bands behave like this in my results. |
@YiminJin Nice work! I won't add another review, but I wanted to ask something. When you say "they all gave the correct result", do you mean the correct result, or just the same result? In #5953 you suggested that this was in fact the incorrect result (or at least different from your elASPECT code). Whatever your answer, I don't think this should impact the merging of this PR. It would just be good to know what will still need fixing after this PR is merged (and why, if that's become clearer since last year). |
@YiminJin thanks for your answer! It's brilliant to see that you have isolated the cause of the difference between elASPECT and ASPECT results. Could you clarify the difference you have identified between "2D models in ASPECT" and "plane-strain models"? It is my understanding that 2D models in ASPECT are explicitly using the plane-strain approximation: https://aspect-documentation.readthedocs.io/en/latest/user/methods/basic-equations/2d-models.html Would it be equivalent to say that one of the "plastic dilation" formulations implicitly assumes isotropic dilation? If so, it is interesting that the two approximations produce such different results, and we should discuss as a larger team what we want in our final implementation. |
There are two functions in ASPECT that are inconsistent with plane strain:
After replacing the two functions by the plane strain version, the shear bands become sharp.
I do not understand this question. Could you specify the "plastic dilation formulation" and the definition of "isotropic dilation" ? (Is there anisotropic dilation?)
Sure! I hope we could make progress together. |
Can you raise a GitHub Issue describing the problem? I agree with your assessment, but it would be good to ask the other developers (a) whether they see something we don't, and (b) how they think we should address the problem given that any change would be a breaking change.
I think we can ignore my original question for the purposes of this PR (because I agree with your plane strain comment). But to answer your question, in plane strain any dilation must be anisotropic, because |
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 will have to continue this review later, but just a few comments for now.
* $\bar\alpha\gamma$ should be added to the right-hand side of the | ||
* mass conservation equation, where $\bar\alpha$ is the negative | ||
* derivative of plastic potential with respect to the pressure | ||
* ($\sin\psi$ in 2D case), and $\gamma$ is the plastic multiplier. | ||
* The plastic multiplier is given by | ||
* $\gamma = (\tau_{II} - \alpha p - k) / \eta^{ve}$, | ||
* where $\tau_{II}$ is the second invariant of the deviatoric stress, | ||
* $\alpha$ is the negative derivative of yield function with respect to | ||
* the pressure ($\sin\phi$ in 2D case), $k$ is cohesion, and $\eta^{ve}$, | ||
* is the pre-yielding viscosity. When the Picard method or Defect Correction | ||
* Method is applied, the term $\bar\alpha\gamma$ should be split into two | ||
* terms: | ||
* $\bar\alpha\gamma = \bar\alpha\alpha p / \eta^{ve} + | ||
* \bar\alpha(\tau_{II} - k) / \eta^{ve}$, | ||
* the former of which should be moved to the left-hand side in order to | ||
* guarantee the stability of the nonlinear solver. Therefore, this output | ||
* provides two terms: dilation_lhs_term corresponds to | ||
* $\bar\alpha\alpha / \eta^{ve}$ (p is replaced by the shape function), | ||
* and dilation_rhs_term corresponds to $(\tau_{II} - k) / \eta^{ve}$. |
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.
All of this description is pretty specific to the case of plastic dilation, but not the general case of someone prescribing delation because of other processes. Maybe it would be better to limit this documentation to the discussion of where the individual terms will go into the equation, and move the specific parts to drucker_prager.h:145
compute_dilation_terms_for_stokes_system(const double angle_friction, | ||
const double angle_dilation, | ||
const double cohesion, | ||
const double non_yielding_viscosity, | ||
const double effective_strain_rate) const; |
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.
could you use the following interface instead, this is more in line with that I did in #6384.
compute_dilation_terms_for_stokes_system(const double angle_friction, | |
const double angle_dilation, | |
const double cohesion, | |
const double non_yielding_viscosity, | |
const double effective_strain_rate) const; | |
compute_dilation_terms_for_stokes_system(const double angle_friction, | |
const double angle_dilation, | |
const double cohesion, | |
const double non_yielding_viscosity, | |
const double effective_strain_rate) const; |
/** | ||
* The LHS term corresponding to plastic dilation in the | ||
* Stokes system. For details, see the comments of | ||
* MaterialModel::PrescribedDilation::dilation_lhs_term. | ||
*/ | ||
std::vector<double> dilation_lhs_terms; | ||
|
||
/** | ||
* The RHS term corresponding to plastic dilation in the | ||
* Stokes system. For details, see the comments of | ||
* MaterialModel::PrescribedDilation::dilation_rhs_term. | ||
*/ | ||
std::vector<double> dilation_rhs_terms; |
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 code part looks different since #6384. Please rebase, the new version will be simpler, because it includes the DruckerPragerParameters
structure already, which will automatically include the dilation angle.
// Average among phases | ||
drucker_prager_parameters.angle_internal_friction = MaterialModel::MaterialUtilities::phase_average_value(phase_function_values, n_phase_transitions_per_composition, | ||
angles_internal_friction, composition); | ||
drucker_prager_parameters.angle_dilation = angles_dilation[composition]; |
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.
wouldnt you also need to use a call to MaterialModel::MaterialUtilities::phase_average_value
to allow different dilation angles for different phases?
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 forgot that. It is corrected in the newest commit.
// Compute the dilation terms if necessary. | ||
if (this->get_parameters().enable_prescribed_dilation == true) | ||
{ | ||
if (output_parameters.composition_yielding[j] == true) | ||
{ | ||
const double current_dilation = drucker_prager_parameters.angle_dilation * weakening_factors[1]; | ||
const std::pair<double,double> dilation_terms = drucker_prager_plasticity.compute_dilation_terms_for_stokes_system ( | ||
current_friction, | ||
current_dilation, | ||
current_cohesion, | ||
non_yielding_viscosity, | ||
effective_edot_ii); | ||
output_parameters.dilation_lhs_terms[j] = dilation_terms.first; | ||
output_parameters.dilation_rhs_terms[j] = dilation_terms.second; | ||
} | ||
else | ||
{ | ||
output_parameters.dilation_lhs_terms[j] = 0; | ||
output_parameters.dilation_rhs_terms[j] = 0; | ||
} | ||
} |
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.
we may need to think where to put this code compared to the code above. @naliboff any suggestions?
Sure. I have raised an issue (#6434 ) about this problem. |
8fda76d
to
35d7af0
Compare
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 addressing my earlier comments. I think this code is already in pretty good shape and almost ready to merge. I left just a few comments concerned with optimizing assembly speed and a few requests for more documentation. Let me know when you have addressed these.
volume_fractions, | ||
dilation_values, | ||
composition_dilation_derivatives_wrt_pressure, | ||
1); |
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.
same here
( one_over_eta + | ||
// add the dilation LHS term if plastic dilation | ||
// is enabled | ||
( prescribed_dilation != nullptr ? | ||
prescribed_dilation->dilation_lhs_term[q] : | ||
0.0 ) | ||
) |
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.
please do this computation up in line 146 where we compute one_over_eta
. E.g.:
const double one_over_eta_plus_dilation = ...
The current loop is one of the most often executed lines in ASPECT, so it is better to move computations and if-branches outside of it.
// consider the derivatives deta/deps | ||
// here, but we leave this as a TODO | ||
one_over_eta | ||
(one_over_eta - dilation_newton_factor) |
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, here you already precomputated dilation_newton_factor
so this can stay as it is.
* (prescribed_dilation->dilation_rhs_term[q] - | ||
prescribed_dilation->dilation_lhs_term[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 is this the difference between rhs and lhs term? Since we are assembling the rhs here, I would expect to only see the rhs side term. Please add a comment that explains this above line 464.
- (prescribed_dilation == nullptr ? 0.0 : | ||
pressure_scaling * pressure_scaling * | ||
prescribed_dilation->dilation_lhs_term[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.
precompute this term outside the loop over i and j.
- (prescribed_dilation == nullptr ? 0.0 : | ||
pressure_scaling * pressure_scaling * | ||
prescribed_dilation->dilation_lhs_term[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.
precompute this term outside the loop over i and j
Assert(!this->get_parameters().enable_prescribed_dilation | ||
|| | ||
outputs.template get_additional_output_object<MaterialModel::PrescribedPlasticDilation<dim>>()->dilation.size() | ||
outputs.template get_additional_output_object<MaterialModel::PrescribedPlasticDilation<dim>>()->dilation_lhs_term.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.
do you need to also add an assert for the dilation_rhs_term
here? just in case someone accidentally sets them to different sizes.
697f16a
to
a832cf4
Compare
@gassmoeller Thank you for the close examination on my codes! The problems you mentioned are addressed 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.
Thank you! This looks good to go to me, but I will leave it open for a day to give @naliboff, @bangerth, and @tjhei the chance to comment if they want to.
Can you also add a changelog entry that describes the changes that users will have to do if they wrote their own dilation terms (i.e. that the variable has been renamed and there are now two terms, lhs and rhs).
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 don't have enough of an understanding of the material models any more to be useful here. I'll let the experts be in charge for this PR :-)
e0b7451
to
b25c571
Compare
b25c571
to
f283ae8
Compare
Since this is blocking #6419 let's merge it. |
This PR is an implementation of plastic dilation in the framework of ASPECT. Currently, plastic dilation is added to the Drucker Prager rheology, but is only available in the
ViscoPlastic
material model. It solves the oscillation problem shown in #5953.I have tested the Picard iteration method, the defect correction method (DCM) and Newton method with the strip-footing experiment, and they all gave the correct result:

The convergence rates of different nonlinear solvers are as expected: Newton > DCM > Picard (either with AMG or with GMG).
VEP model with dilation angle ($\psi$ ) identical to the friction angle ($\phi$ ) has two distinctive features:
The features can be observed in the numerical results of Kaus' (2010) benchmark (without viscoplastic damper):

The reason of feature 1 has been widely discussed and verified, while the reason of feature 2 is not so clear (the theory of associated/non-associated plastic flow can explain it, but I do not fully understand the theory).
I am sorry for modifying over 1000 lines of codes in a single PR. Please let me know if you find something confusing in my codes.