Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added isostress averaging to CompositeViscoPlastic #5990
added isostress averaging to CompositeViscoPlastic #5990
Changes from all commits
b1e55a0
4cc572a
82c7607
f41a86e
791ea55
84ab5db
56a8c6a
2f09e15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
actually, why is this function and one above (
compute_composition_viscosity
) public? they are also just called from inside the rheology model?And can you explain the name change? Is it to align this function with the
compute_composition_viscosity
? From the description it is not obvious what connects this function to acomposition
. For symmetry reasons withcalculate_isostress_log_strain_rate_and_derivative
would it be correct to call this functioncalculate_isostrain_log_strain_rate_and_derivative
(and similar for thecompute_composition_viscosity
function)?From a structure perspective these are the function names I would find cleanest (but I will let you comment if this makes sense):
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'd also like this symmetry, but I don't see a way to do it neatly.
The problem: while the isostress viscosity solves for a single stress, the isostrain rheology solves for one stress per compositional field. It felt natural to create inner functions that only do one solve, leading to the two functions for "composition" (used several times by the isostrain branch) and the two functions for "isostress" (only used once by the isostress branch).
I'm happy to group the loop over compositions into a
compute_isostrain_viscosity
if you think that would be neater, but that would still leave acalculate_composition_log_strain_rate_and_derivative
.What would you prefer?
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 the explanation, I understand now. Yes outsourcing the loop over compositions into a
compute_isostrain_viscosity
and then mentioning in thecalculate_composition...
functions that they are only computing things for one composition would be most intuitive I think. What confused me most about the current situation is that there wascompute_isostress
but nocompute_isostrain
, but insteadcompute_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.
Great, I've now made a new compute_isostrain_viscosity function. I agree that this makes more logical sense.