-
Notifications
You must be signed in to change notification settings - Fork 240
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
Fix elastic stress used in principal_stress postprocessor #5480
Fix elastic stress used in principal_stress postprocessor #5480
Conversation
ce7921a
to
54f3af7
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.
The postprocessor is documented as follows:
"A visualization output object that outputs the "
"principal stress values and directions, i.e., the "
"eigenvalues and eigenvectors of the stress tensor. "
"The postprocessor can either operate on the full "
"stress tensor or only on the deviatoric stress tensor, "
"depending on what run-time parameters are set."
I think it would be useful to define what "principal stress" actually means here, so that we can reason what the postprocessor should actually do. As it stands, it's hard to tell whether what you do is correct or not because we haven't defined what this postprocessor should actually do.
if (this->get_parameters().enable_elasticity == true) | ||
{ | ||
stress[0][0] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xx")]; | ||
stress[1][1] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yy")]; | ||
stress[0][1] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xy")]; | ||
stress[0][0] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xx")]; | ||
stress[1][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yy")]; | ||
stress[0][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xy")]; | ||
|
||
if (dim == 3) | ||
{ | ||
stress[2][2] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_zz")]; | ||
stress[0][2] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xz")]; | ||
stress[1][2] += in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yz")]; | ||
stress[2][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_zz")]; | ||
stress[0][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xz")]; | ||
stress[1][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yz")]; | ||
} | ||
} | ||
else | ||
{ | ||
const SymmetricTensor<2,dim> deviatoric_strain_rate | ||
= (this->get_material_model().is_compressible() | ||
? | ||
in.strain_rate[q] - 1./3. * trace(in.strain_rate[q]) * unit_symmetric_tensor<dim>() | ||
: | ||
in.strain_rate[q]); | ||
|
||
// Compressive stress is positive in geoscience applications | ||
stress = -2. * out.viscosities[q] * deviatoric_strain_rate; | ||
} |
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.
Would you mind switching the if and else branches here? Most readers will approach this from the non-elasticity case in mind, and it would be nice to see the non-elastic case first.
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 also having the commented note about sign convention (the need to convert from the convention used inside of the code to that used in geology and in post processing here) moved above both the viscous and viscoelastic cases would make it clearer what is being done
stress[0][0] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xx")]; | ||
stress[1][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yy")]; | ||
stress[0][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xy")]; |
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 sign should be changed here similarly in how it is changed for the viscous case in line 144 to convert from the convention used inside of the code to the geologic convention of compression as positive
stress[0][0] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xx")]; | |
stress[1][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yy")]; | |
stress[0][1] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xy")]; | |
stress[0][0] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xx")]; | |
stress[1][1] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yy")]; | |
stress[0][1] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xy")]; |
stress[2][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_zz")]; | ||
stress[0][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xz")]; | ||
stress[1][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yz")]; |
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.
stress[2][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_zz")]; | |
stress[0][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xz")]; | |
stress[1][2] = in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yz")]; | |
stress[2][2] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_zz")]; | |
stress[0][2] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_xz")]; | |
stress[1][2] = - in.composition[q][this->introspection().compositional_index_for_name("ve_stress_yz")]; |
@gassmoeller I have tested this and found it does not fix the stress orientation issue in the hanging beam test ( tests and description on this thread: https://community.geodynamics.org/t/principal-stress-orientations-in-viscous-vs-visco-elastic-models/3161/5). I made some comments to your code changes above where I think the remaining issue is. |
54f3af7
to
fe7d12d
Compare
All good points, thanks for the review. I have corrected the signs and added explanation to the documentation of the postprocessor. The problem with the sign is really that it is a convention, and we just need to be consistent to use the same convention. |
@gassmoeller You can incorporate those test cases with this PR. I think that makes sense so it is all together! (I also was having some technical problems on my end and don't want to hold up the merging) |
ec1f3bb
to
9efbd64
Compare
9efbd64
to
b8442ef
Compare
Ok, I added your tests to this PR and made sure the results are similar to the ones you posted in the forum. This fix should be ready for a final review and merge. |
One of the testers is stuck, but since the others are all succeeding, I'll take the liberty to merge. |
Similar to #4375, but for the principal_stress postprocessor.
@rfildes: Could you check if this change also fixes the bug you observed?
@bobmyhill or @anne-glerum: Could one of you review this PR?