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
Add compositional dependence on viscosity in the Steinberger material model #5260
Add compositional dependence on viscosity in the Steinberger material model #5260
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.
@poulamiiroy - Thank you for the contribution and breaking the original PR into smaller contributions! I think with a few moderate adjustments the PR will be in good shape. Please let me know if you have any questions about my comments, and thank you again for the contribution!
@@ -274,6 +274,7 @@ namespace aspect | |||
std::vector<double> conductivity_reference_temperatures; | |||
std::vector<double> conductivity_exponents; | |||
std::vector<double> saturation_scaling; | |||
std::vector<double> prefactors; |
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.
std::vector<double> prefactors; | |
std::vector<double> viscosity_prefactors; |
Can you also move this variable down a few lines and add documentation that described what it represents. The variable is currently lumped with other variables related to thermal conductivity.
source/material_model/steinberger.cc
Outdated
const SymmetricTensor<2,dim> &, | ||
const Point<dim> &position) const | ||
{ | ||
const double depth = this->get_geometry_model().depth(position); | ||
const double adiabatic_temperature = this->get_adiabatic_conditions().temperature(position); | ||
// Calculate volume fractions from mass fractions |
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.
Can the following four lines be removed?
@@ -189,11 +194,14 @@ namespace aspect | |||
// For an explanation on this formula see the Steinberger & Calderwood 2006 paper | |||
const double vis_lateral_exp = -1.0*lateral_viscosity_lookup->lateral_viscosity(depth)*delta_temperature/(temperature*adiabatic_temperature); | |||
// Limit the lateral viscosity variation to a reasonable interval | |||
|
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 would remove this extra line, and instead add it between lines 195 and 196.
source/material_model/steinberger.cc
Outdated
const double vis_lateral = std::max(std::min(std::exp(vis_lateral_exp),max_lateral_eta_variation),1/max_lateral_eta_variation); | ||
|
||
const double vis_radial = radial_viscosity_lookup->radial_viscosity(depth); | ||
const double vis_compositional = MaterialUtilities::average_value (volume_fractions, prefactors, viscosity_averaging); |
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.
const double vis_compositional = MaterialUtilities::average_value (volume_fractions, prefactors, viscosity_averaging); | |
// Volume fraction averaged value of the composition-dependent viscosity prefactors | |
const double vis_prefactor = MaterialUtilities::average_value (volume_fractions, prefactors, viscosity_averaging); |
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 wanted to keep it as vis_compositional since I am already renaming "prefactors" as "viscosity_prefactors" as you have suggested. What do you think?
source/material_model/steinberger.cc
Outdated
@@ -268,11 +276,16 @@ namespace aspect | |||
|
|||
// Evaluate the equation of state properties over all evaluation points | |||
equation_of_state.evaluate(eos_in, eos_outputs); | |||
// std::vector<Vector<double>> viscosities(in.n_evaluation_points(), this->n_compositional_fields()); |
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.
Can you remove this and additional commented lines below if they are no longer needed?
source/material_model/steinberger.cc
Outdated
|
||
return std::max(std::min(vis_lateral * vis_radial,max_eta),min_eta); | ||
return std::max(std::min(vis_lateral * vis_radial * vis_compositional,max_eta),min_eta); |
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 confess I don't understand this equation, as it looks like viscosity values are being multiplied together. Would it be possible to add the equation here (or further above) in a comment, along with a brief description?
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.
Agreed, as written it looks like multiplication of different viscosities. Would you mind renaming
vis_lateral
totemperature_prefactor
vis_compositional
tocompositional_prefactor
vis_radial
toeta_ref
?
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 second @bobmyhill renaming scheme for the values
source/material_model/steinberger.cc
Outdated
"geometric, or maximum composition."); | ||
prm.declare_entry ("Prefactors", "1.e21", | ||
Patterns::Anything(), | ||
"List of viscosities for background mantle and compositional fields," |
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.
Just to be sure, are the "Prefactors" in fact viscosity values, or are they dimensionless composition-dependent factors that the viscosity is multiplied by?
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.
Follow up - depending on the answer, I would adjust the parameter name and description to be a bit more specific.
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.
Prefactors are the dimensionless composition-dependent factors, for example, if one composition needs to have 10 times higher viscosity than ambient, then prefactor for this composition would be 10.
source/material_model/steinberger.cc
Outdated
|
||
viscosity_averaging = MaterialUtilities::parse_compositional_averaging_operation ("Viscosity averaging scheme", | ||
prm); | ||
// viscosities = Utilities::parse_map_to_double_array (prm.get("Viscosities"), |
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.
Remove commented lines?
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 a lot John for reviewing my pull request. I will follow what you have suggested. Please let me know for further discussion.
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 looks goof to me, thank you for contributing this! Can you address the few small comments I left, and then I think this is good to be merged!
source/material_model/steinberger.cc
Outdated
// if (in.requests_property(MaterialProperties::viscosity)) | ||
// out.viscosities[i] = viscosity(in.temperature[i], in.pressure[i], in.composition[i], in.strain_rate[i], in.position[i]); |
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.
If these lines are not needed anymore, simply remove them.
source/material_model/steinberger.cc
Outdated
// out.viscosities[i] = MaterialUtilities::average_value (volume_fractions[i], viscosities, viscosity_averaging); | ||
// out.viscosities[i] = viscosities[i]; |
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.
Same here, these lines can be removed.
source/material_model/steinberger.cc
Outdated
"model. Larger values will be cut off."); | ||
"model. Larger values will be cut off."), |
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 do not think you want to change this. This actually needs to be a semicolon.
source/material_model/steinberger.cc
Outdated
Patterns::Selection("arithmetic|harmonic|geometric|maximum composition"), | ||
"When more than one compositional field is present at a point " | ||
"with different viscosities, we need to come up with an average " | ||
"viscosity at that point. Select a weighted harmonic, arithmetic, " |
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.
"viscosity at that point. Select a weighted harmonic, arithmetic, " | |
"viscosity at that point. Select a weighted harmonic, arithmetic, " |
source/material_model/steinberger.cc
Outdated
// viscosities = Utilities::parse_map_to_double_array (prm.get("Viscosities"), | ||
// list_of_composition_names, | ||
// has_background_field, | ||
// "Viscosities"); |
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.
// viscosities = Utilities::parse_map_to_double_array (prm.get("Viscosities"), | |
// list_of_composition_names, | |
// has_background_field, | |
// "Viscosities"); |
I think John is just asking to remove these lines.
tests/multicomponent_steinberger.prm
Outdated
subsection Steinberger model | ||
set Use lateral average temperature for viscosity = false | ||
set Data directory = $ASPECT_SOURCE_DIR/data/material-model/steinberger/ | ||
# set Derivatives file names = /home/poulami/software/aspect_30_09_2022/aspect/data/material-model/steinberger/test-steinberger-compressible/constant_material_small_hefesto_derivatives.txt |
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.
If you're not using that line, simply remove it.
tests/multicomponent_steinberger.prm
Outdated
set Use lateral average temperature for viscosity = false | ||
set Data directory = $ASPECT_SOURCE_DIR/data/material-model/steinberger/ | ||
# set Derivatives file names = /home/poulami/software/aspect_30_09_2022/aspect/data/material-model/steinberger/test-steinberger-compressible/constant_material_small_hefesto_derivatives.txt | ||
set Radial viscosity file name = radial-visc.txt #, radial-visc.txt, radial-visc.txt ## lithosphere at top |
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.
set Radial viscosity file name = radial-visc.txt #, radial-visc.txt, radial-visc.txt ## lithosphere at top | |
set Radial viscosity file name = radial-visc.txt # lithosphere at top |
Same here, remove everything that is not used.
tests/multicomponent_steinberger.prm
Outdated
set Data directory = $ASPECT_SOURCE_DIR/data/material-model/steinberger/ | ||
# set Derivatives file names = /home/poulami/software/aspect_30_09_2022/aspect/data/material-model/steinberger/test-steinberger-compressible/constant_material_small_hefesto_derivatives.txt | ||
set Radial viscosity file name = radial-visc.txt #, radial-visc.txt, radial-visc.txt ## lithosphere at top | ||
set Lateral viscosity file name = temp-viscosity-prefactor.txt ##, temp-viscosity-prefactor.txt, temp-viscosity-prefactor.txt |
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.
set Lateral viscosity file name = temp-viscosity-prefactor.txt ##, temp-viscosity-prefactor.txt, temp-viscosity-prefactor.txt | |
set Lateral viscosity file name = temp-viscosity-prefactor.txt |
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.
It does not look like you are actually using this file. Is that intentional? If you do not want to use it in your test case below, simply remove it from your pull request.
source/material_model/steinberger.cc
Outdated
//The lateral variation of viscosity due to lateral temperature (Visc_lT) | ||
//Visc_lT = exp [(-1)*(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT)], Eq. 6 of the paper |
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 what @naliboff was asking for was a more detailed explanation of why these things are multiplied together. I have made a suggestion below:
//The lateral variation of viscosity due to lateral temperature (Visc_lT) | |
//Visc_lT = exp [(-1)*(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT)], Eq. 6 of the paper | |
// We here compute the lateral variation of viscosity due to temperature (vis_lateral) as | |
// V_lT = exp [-(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT))] as in Eq. 6 of the paper. | |
// We get H/nR from the lateral_viscosity_lookup->lateral_viscosity function. |
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.
@jdannberg Thanks a lot for your review! I made the changes.
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.
It looks like many of the tests that use the steinberger material model are failing now. They all have the error message
An error occurred in line <943> of file </__w/aspect/aspect/source/material_model/utilities.cc> in function
double aspect::MaterialModel::MaterialUtilities::average_value(const std::vector<double, std::allocator<double> >&, const std::vector<double, std::allocator<double> >&, const aspect::MaterialModel::MaterialUtilities::CompositionalAveragingOperation&)
The violated condition was:
volume_fractions.size() == parameter_values.size()
Additional information:
The volume fractions and parameter values vectors used for averaging
have to have the same length!
To figure out what might be the problem, I would suggest to run these tests locally on your machine. For example, you could run them in the debugger to see what the two values are (volume fractions and viscosity prefactors) and which of these actually has the wrong length.
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.
You will need to update this test output with the one from the official tester (and same for the statistics file). The file that you added here still, for example, includes the output path on your own computer (Writing graphical output: /home/poulami/software/Aspect_globalscale/aspect/tests/multicomponent_steinberger/solution/solution-00001
)
/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.
A couple of suggested changes to variable names that will make code comprehension easier.
source/material_model/steinberger.cc
Outdated
|
||
return std::max(std::min(vis_lateral * vis_radial,max_eta),min_eta); | ||
return std::max(std::min(vis_lateral * vis_radial * vis_compositional,max_eta),min_eta); |
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.
Agreed, as written it looks like multiplication of different viscosities. Would you mind renaming
vis_lateral
totemperature_prefactor
vis_compositional
tocompositional_prefactor
vis_radial
toeta_ref
?
source/material_model/steinberger.cc
Outdated
@@ -187,13 +189,22 @@ namespace aspect | |||
delta_temperature = temperature-adiabatic_temperature; | |||
|
|||
// For an explanation on this formula see the Steinberger & Calderwood 2006 paper | |||
// We here compute the lateral variation of viscosity due to temperature (vis_lateral) as | |||
// V_lT = exp [-(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT))] as in Eq. 6 of the paper. | |||
// We get H/nR from the lateral_viscosity_lookup->lateral_viscosity function. | |||
const double vis_lateral_exp = -1.0*lateral_viscosity_lookup->lateral_viscosity(depth)*delta_temperature/(temperature*adiabatic_temperature); |
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.
Please rename vis_lateral_exp
to log_temperature_prefactor
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 follow-up question on these lines.
The equation in the comments is written as V_lT = exp [-(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT))]
, while the actual equation on line 195 has delta_temperature/(temperature*adiabatic_temperature)
.
Is the equation in the comments referring to the same equation as the one on line 195? If yes, can you confirm which one is correct or expand the documentation a bit further to explain the differences between the two equations?
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.
@naliboff I suggested to write the equation like this, this is how it is written in the Steinberger and Calderwood paper.
temperature
is the same as T_adiabatic + dT
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.
@poulamiiroy - A few additional comments on top of those from @bobmyhill.
source/material_model/steinberger.cc
Outdated
|
||
return std::max(std::min(vis_lateral * vis_radial,max_eta),min_eta); | ||
return std::max(std::min(vis_lateral * vis_radial * vis_compositional,max_eta),min_eta); |
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 second @bobmyhill renaming scheme for the values
source/material_model/steinberger.cc
Outdated
@@ -187,13 +189,22 @@ namespace aspect | |||
delta_temperature = temperature-adiabatic_temperature; | |||
|
|||
// For an explanation on this formula see the Steinberger & Calderwood 2006 paper | |||
// We here compute the lateral variation of viscosity due to temperature (vis_lateral) as | |||
// V_lT = exp [-(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT))] as in Eq. 6 of the paper. | |||
// We get H/nR from the lateral_viscosity_lookup->lateral_viscosity function. | |||
const double vis_lateral_exp = -1.0*lateral_viscosity_lookup->lateral_viscosity(depth)*delta_temperature/(temperature*adiabatic_temperature); |
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 follow-up question on these lines.
The equation in the comments is written as V_lT = exp [-(H/nR)*dT/(T_adiabatic*(T_adiabatic + dT))]
, while the actual equation on line 195 has delta_temperature/(temperature*adiabatic_temperature)
.
Is the equation in the comments referring to the same equation as the one on line 195? If yes, can you confirm which one is correct or expand the documentation a bit further to explain the differences between the two equations?
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.
Okay, the code looks much better now and most of the tests are succeeding as well! 👍
But there is still one test that is failing (steinberger_phase_fractions_background). The test finishes now, but the results are different, so you'll need to figure out why the output is different (i.e. which column in the graphical output is different? Is it the viscosity? What has changed compared to before?).
In addition, you have accidentally added some lines in your last commit that are just printing statements you commented out.
// std::cout<<"size vf"<<volume_fractions.size()<<std::endl; | ||
// std::cout<<"size vis_pre"<<viscosity_prefactors.size()<<std::endl; |
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.
You added some lines again here that are code you commented out and that is no longer needed. Please remove them.
// std::cout<<"size vf"<<volume_fractions.size()<<std::endl; | ||
// std::cout<<"size vis_pre"<<viscosity_prefactors.size()<<std::endl; | ||
const double compositional_prefactor = MaterialUtilities::average_value (volume_fractions, viscosity_prefactors, viscosity_averaging); | ||
// std::cout<<"size vf"<<volume_fractions.size()<<std::endl; |
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.
Same here, please remove this line.
@@ -303,7 +305,7 @@ namespace aspect | |||
chemical_compositions.push_back(in.composition[i][c]); | |||
|
|||
mass_fractions = MaterialUtilities::compute_composition_fractions(chemical_compositions, *composition_mask); | |||
|
|||
// std::cout<<"mf"<<mass_fractions<<std::endl; |
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.
Please remove this line as well.
@poulamiiroy Ignore my last; thank you for making the variable name changes we suggested. |
@@ -551,7 +580,15 @@ namespace aspect | |||
+ " fields of type chemical composition.")); | |||
|
|||
has_background_field = (equation_of_state.number_of_lookups() == n_chemical_fields + 1); | |||
|
|||
std::vector<std::string> list_of_composition_names = this->introspection().get_composition_names(); |
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 you want fields that correspond to chemical compositions, rather than all compositional fields:
std::vector<std::string> list_of_composition_names = this->introspection().get_composition_names(); | |
std::vector<std::string> list_of_composition_names = this->introspection().chemical_composition_field_names(); |
This might fix a couple of the tests.
if (equation_of_state.number_of_lookups() == 1) | ||
list_of_composition_names.resize(1, "background"); | ||
if ((equation_of_state.number_of_lookups() == 1) && (has_background_field)) | ||
list_of_composition_names.resize(0, "background"); |
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.
Are you sure that these lines cover all the potential use cases of the material model?
The way you have implemented this, if the number of lookups is equal to 1, the vector of composition names is replaced by {"background"}
. If additionally a background field is implied, the vector is empty {}
.
What happens when the number of lookups is not equal to 1? You should test that parse_map_to_double_array
does the correct thing in these cases.
You might be also be interested in mirroring the changes I made here, using the new map parser:
https://github.com/geodynamics/aspect/pull/5039/files#diff-cfe301f322bebee87432e53137677e6372743e5e142733d8fb63b5599c9322cb
Thank you for your comments! I am working on it. I'll ping you when the PR needs a new review. :) |
source/material_model/steinberger.cc
Outdated
// if ((equation_of_state.number_of_lookups() > 1) && (has_background_field)) | ||
// list_of_composition_names.resize(equation_of_state.number_of_lookups(), "background"); | ||
if (equation_of_state.number_of_lookups() > 1) | ||
list_of_composition_names.resize(n_chemical_fields, "background"); |
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 line that you've added here appends to the list_of_composition_names vector with entries called "background" (e.g. {"a", "b", "background", "background", "background", "background", ...}.
I doubt that is what you want to do. The background field is the first compositional field, not the last. And there is only ever one of them.
Have another look at the link above. I suspect "insert" will be more useful to you than "resize".
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.
If you're ever unsure of what a C++ function does, you can always look it up: https://cplusplus.com/reference/vector/vector/resize/
https://cplusplus.com/reference/vector/vector/insert/
Closed in favour of #5362 |
This pull request supersedes the PR #5248.
Added compositional dependence on viscosity in the Steinberger material model. The original Steinberger material model uses one viscosity profile for all of the compositions. So, I have added prefectors which are multiplied by original viscosity based on volume fraction.
For all pull requests:
I have followed the instructions for indenting my code.
I have tested my new feature locally to ensure it is correct.
I have created a testcase for the new feature/benchmark in the tests/ directory.
I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.