-
Notifications
You must be signed in to change notification settings - Fork 241
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
Be safer and more correct when using strain rate #5239
Be safer and more correct when using strain rate #5239
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.
Looks sensible. One grammatical suggestion.
@@ -218,9 +218,11 @@ namespace aspect | |||
out.entropy_derivative_temperature[i] = MaterialUtilities::average_value (volume_fractions, eos_outputs.entropy_derivative_temperature, MaterialUtilities::arithmetic); | |||
|
|||
// Compute the effective viscosity if requested and retrieve whether the material is plastically yielding |
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.
// Compute the effective viscosity if requested and retrieve whether the material is plastically yielding | |
// Compute the effective viscosity if requested and retrieve whether the material is plastically yielding. |
1b37bf5
to
b1a9785
Compare
Specifically, the single failing test is:
Isn't that test new? Did that test succeed on your branch before the commits? |
if (in.requests_property(MaterialProperties::viscosity)) | ||
if (in.requests_property(MaterialProperties::viscosity) || in.requests_property(MaterialProperties::additional_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.
This seems right, but points out the fact that we should add an else
to this if
that poisons the output fields then not computed with signaling_nan
s.
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.
@bangerth - are the material model outputs by default not already set to signaling nans?
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.
Perhaps -- we weren't sure whether it's possible that the same object was reused from a state where information was requested to where it is no longer requested (but at that point no longer contains the signaling nans).
source/simulator/assembly.cc
Outdated
need_viscosity); | ||
/*compute_strain_rate=*/ true); |
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.
Doesn't this negate the point of the flag? Do you even still need need_viscosity
in this case?
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 still need the flag, because need_viscosity
is used in the next line to tell the material model which outputs to compute. The wrong assumption here was that if we do not explicitly request the viscosity we also never need the strain rate input. This is not true anymore for complex material model (strain rate may be used for other properties than just viscosity).
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.
Ah, that's a bummer. I recall that computing the strain rate was expensive, and it's a shame that we now compute it unconditionally because the material model does not have a way to indicate whether or not it actually wants it...
Since you observe that this patch changes output, the conclusion is that apparently we forgot to compute something previously and used invalid values (perhaps zeros, perhaps computed previously on the same object). This suggests what I mention in a comment above: We should have poisoned these values in an |
See #5247 for an attempt. |
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.
@gassmoeller - Aside from addressing comments from @bangerth, this looks good to go. Thanks for adding this.
One general comment - at some point it may be worth adding the checks for using the reference strain rate to the material model utilities, as other material models have to do the same checks.
22ba34c
to
d555941
Compare
d555941
to
7955a55
Compare
This PR contains three changes that make using the strain rate in material models safer. All of these changes were necessary for #5211.