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

fixed stress visualization for elastic materials #4375

Merged
merged 7 commits into from
Nov 30, 2023

Conversation

bobmyhill
Copy link
Member

@bobmyhill bobmyhill commented Oct 1, 2021

This PR (hopefully) fixes the stress visualization for materials where elasticity is enabled. The ve_stress_xx compositional fields correspond to the full deviatoric stress, not only additional elastic stresses.

Opened for testing.

@bobmyhill
Copy link
Member Author

/rebuild

@anne-glerum
Copy link
Contributor

anne-glerum commented Oct 4, 2021

Hi @bobmyhill , thanks for the fix! Two comments:

  • no test broke, is there no test with elasticity and the stress visu output? Perhaps you can add the visu output to an existing test?
  • there are currently 2 other visu postprocessors that compute the stress and take elasticity into account:
    • principle_stresses.cc
    • shear_stress.cc (which I've been meaning to fix, I think it's the deviatoric stress and the elastic stresses should not be added as a last step).
      Could you make the same changes there?

@bobmyhill
Copy link
Member Author

Hi @anne-glerum, sure, I can make those changes.
Can we use one of your tests (i.e. one where we know what the output should look like, and which is broken in the existing code)?

@anne-glerum
Copy link
Contributor

Great. If #4369 is merged, you could add the stress postprocessor to the viscoelastic_stress_build-up test (or stick with the stress invariant postprocessor already selected). The value of ve_stress_xx should be 100 MPa, ve_stress_yy -100 MPa and of the second invariant 100 MPa.

@naliboff
Copy link
Contributor

@bobmyhill - I agree with @anne-glerum that the proposed changes look good and we should merge after the requested changes to principal_stresses.cc and shear_stress.cc are are done (either here or in a separate PR). If it would help, I can make separate PRs for those other post processors, which can then be merged after this PR is merged.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Ouch, it looks like this bugfix was ready all the time and we forgot to merge it. Let's do this now.

@bobmyhill or @anne-glerum: Could you check the status of the other postprocessors mentioned in your discussion. Are they already fixed, or also still broken? If they also have waiting PRs, could you point me to them?

@gassmoeller
Copy link
Member

/rebuild

@bobmyhill
Copy link
Member Author

@gassmoeller I've now added the shear stress postprocessor and a test.
I'm confused by the test results - https://raw.githubusercontent.com/geodynamics/aspect/3391ba82148cb77f8cbf05c072321f5564235cb3/tests/viscoelastic_stress_build-up/solution/solution-00000.0000.gnuplot
The stress_xx, stress_yy and stress_xy fields look as expected, but the shear stress fields look like they either have the wrong sign, or are in the wrong order. I would have expected them to have the order xx, xy, yx, yy.

@bobmyhill
Copy link
Member Author

Ah, I misunderstood the output. stress_xx, stress_xy and stress_yy are output directly from the fields, so they have the opposite sign to the ones calculated using the postprocessors.

This looks a bit weird, but I guess we have to put up with it.

@bobmyhill
Copy link
Member Author

One thing that I still need to double check and document is the sign convention for the initial conditions for the stress fields.

@bobmyhill bobmyhill removed the request for review from naliboff November 11, 2023 10:28
@bobmyhill
Copy link
Member Author

@anne-glerum, @gassmoeller, @tjhei: This should now be ready for a ?final round of review.

Philosophical question: at the moment, users input initial stresses with the engineering sign convention (See, for example, aspect/tests/viscoelastic_stress_build-up.prm), but we output with the geological sign convention. Not something to deal with in this PR, but I do think we should make the user-facing bits of ASPECT consistent. The easiest way to do this would be to output stresses with the engineering convention.

@bobmyhill
Copy link
Member Author

Hey @gassmoeller, could you look this over now that your PR (#5480) is merged?

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Thanks for the reminder. Almost good to go if you address my nitpicking :-)

source/postprocess/visualization/shear_stress.cc Outdated Show resolved Hide resolved
source/postprocess/visualization/shear_stress.cc Outdated Show resolved Hide resolved
source/postprocess/visualization/stress.cc Outdated Show resolved Hide resolved
source/postprocess/visualization/stress.cc Show resolved Hide resolved
@bobmyhill
Copy link
Member Author

Nitpicking addressed :)

@bobmyhill
Copy link
Member Author

Looks like the test failures are unrelated to this PR (gmg related). See prebuild tests here: https://github.com/geodynamics/aspect/actions/runs/7038002009/job/19156376251?pr=4375#logs

@gassmoeller
Copy link
Member

Yup, the test failures are unrelated. This is good to go. Thanks for the patience.

@gassmoeller gassmoeller merged commit e6eadff into geodynamics:main Nov 30, 2023
4 of 5 checks passed
@bobmyhill bobmyhill deleted the fix_vep_stress_viz branch November 30, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants