-
Notifications
You must be signed in to change notification settings - Fork 251
Create new viscosity prefactor feature #5678
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
Create new viscosity prefactor feature #5678
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.
Hi @danieldouglas92, thanks for doing this!
The code structure is what I had envisaged, but the content of ViscosityPrefactors<dim>::compute_viscosity(...)
is unexpected to me. I think what you want is to vary your viscosity as a function of the values of the compositional fields, right? If so, the viscosity should not be an explicit function of time.
My thought was that compute_viscosity would have different / additional arguments compared to the function in constant_viscosity_prefactors.cc
. For example, you could have base_viscosity
and a double or vector-double of state_parameters
, and then have the function calculate the new viscosity based on the values of those state parameters. I think you just need a power law, right?
To start with, maybe just code up the functional form that you will need for your scientific case; we can extend in later PRs as 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.
@bobmyhill - here is a summary of my conversation with @danieldouglas92 about this PR.
-
The long-term goal is to have a single rheology module that supports multiple cases for handling prefactor terms that are multiplied with the viscous flow laws.
-
The way for selecting this could be through enum variable, similar to
ViscosityScheme
inrheology/visco_plastic
enum ViscosityPrefactorSheme { constant, mantle_hydration } viscosity_prefactor_scheme;
with more schemes added on through time.
- As a starting point, this PR could be modified to just re-implement the constant scheme and re-use the same test? Or perhaps it would be better to add in a slightly different (but simple to implement) test case so that we are not re-implementing a constant viscosity prefactor multiplication case in
visco_plastic
?
e904352
to
12fd230
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.
This is looking good, thanks! Mainly comments on documentation and naming schemes.
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
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.
@danieldouglas92 - Here are a few additional comments on the test case.
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
ee4a742
to
dbc0fad
Compare
This should be ready for another round of reviews @naliboff and @bobmyhill, thanks in advance! |
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.
Nice! I think this is almost there, just a few comments.
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
dbc0fad
to
8c3bdf9
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.
A few more minor renaming suggestions
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
Should be ready for another run through @naliboff |
0436540
to
65b032a
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.
@danieldouglas92 - Thanks for the updates and the PR is in great shape! Only a few minor cleanup requests below.
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
tests/hk04_olivine_dislocation_hydration_prefactor/solution/solution-00000.0000.vtu
Outdated
Show resolved
Hide resolved
tests/hk04_olivine_dislocation_hydration_prefactor/solution/solution-00000.pvtu
Outdated
Show resolved
Hide resolved
tests/hk04_olivine_dislocation_hydration_prefactor/solution/solution-00000.visit
Outdated
Show resolved
Hide resolved
65b032a
to
2728257
Compare
As long as the testers pass, I think I just need one last review from you @bobmyhill! |
2728257
to
6b6c4cd
Compare
/rebuild |
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.
Nice! Just a few doc comments and then this is good to go.
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
include/aspect/material_model/rheology/compositional_viscosity_prefactors.h
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
source/material_model/rheology/compositional_viscosity_prefactors.cc
Outdated
Show resolved
Hide resolved
Ready for another (hopefully last) run through @naliboff and @bobmyhill! Thanks again for all the helpful feedback |
1ef8694
to
e4edbed
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.
Good to go from my side, thanks!
54fa1a0
to
1edef1e
Compare
1edef1e
to
c9d866d
Compare
This pull request implements the framework that will allow the user to apply more complicated viscosity prefactors to the visco-plastic material model, like accounting for the water fugacity. This is what I implement here. The compositional field 'bound_water' is queried and used to determine the water fugacity for olivine using Hirth and Kohlstaedt 2004 (10.1029/138GM06). This should be set up in such a way that it can be applied to the other rheologies, like diffusion/dislocation creep. This PR takes the place of #4965