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

Rheology and conductivity modification for the entropy plugin #5254

Merged
merged 1 commit into from Jul 13, 2023

Conversation

RanpengLi
Copy link
Contributor

@RanpengLi RanpengLi commented Jul 10, 2023

I added the following features to the current entropy plugin

  • plasticity controlled by cohesion and frictional angle
  • lateral variation limit of viscosity from the reference profile
  • depth dependent prefactor profile for lateral viscosity change
  • selecting between constant and p-T dependent thermal conductivity

For all pull requests:

For new features/models or changes of existing features:

  • 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.

@gassmoeller
Copy link
Member

/rebuild

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 this looks good, but I have few small comments. A bigger comment is that you can probably remove the LateralViscosityLookup class and use the class from the steinberger.h directly, without copying the code. Let me know when you have addressed the comments.

Also please add a changelog entry in the doc/modules/changes, see here for a description: https://github.com/geodynamics/aspect/blob/main/CONTRIBUTING.md#changelog-entries


namespace aspect
{
namespace MaterialModel
{
using namespace dealii;

namespace internal
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to add #include <aspect/material_model/steinberger.h> at the top of the file, then you can delete this class declaration here. This does only work if you did not change something in the implementation of the LaterialViscosityLookup class.

/**
* Minimum/Maximum viscosity and lateral viscosity variations.
*/
double lateral_viscosity_prefactor;
//double lateral_viscosity_prefactor;
Copy link
Member

Choose a reason for hiding this comment

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

remove this line


namespace aspect
{
namespace MaterialModel
{
namespace internal
Copy link
Member

Choose a reason for hiding this comment

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

If you use the same class as the steinberger material model you can delete all of this copy (as long as you #include <aspect/material_model/steinberger.h at the top).

}



Copy link
Member

Choose a reason for hiding this comment

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

Keep this empty line. In fact our convention is to have three empty lines between functions in .cc files (and one empty line in .h files).

const double p_dependence = reference_thermal_conductivities[layer_index] + conductivity_pressure_dependencies[layer_index] * pressure;

// Make reasonably sure we will not compute any invalid values due to the temperature-dependence.
// Since both the temperature-dependence and the saturation term scale with (Tref/T), we have to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Since both the temperature-dependence and the saturation term scale with (Tref/T), we have to
// Since both the temperature-dependence and the saturation time scale with (Tref/T), we have to

Copy link
Member

Choose a reason for hiding this comment

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

time scalehas not been addressed

Comment on lines 294 to 303
// prm.declare_entry ("Lateral viscosity prefactor", "0.0",
// Patterns::Double(0),
// "The lateral viscosity prefactor used for computing the temperature"
// "dependence of viscosity. This corresponds to H/nR (where H is the "
// "activation enthalpy, n is the stress exponent and R is the gas "
// "constant) in an Arrhenius-type viscosity law, and is modeled after "
// "the law for lateral viscosity variations given in Steinberger and "
// "Calderwood, 2006. "
// "\n\n"
// "Units: $1/K$");
Copy link
Member

Choose a reason for hiding this comment

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

Remove all commented source code. We never merge commented code, because it will be impossible in the future to decide if this is is outdated code, or if it is code to be fixed and included in the future.

// "Units: $1/K$");
prm.declare_entry ("Lateral viscosity prefactor file name", "temp-viscosity-prefactor.txt",
Patterns::Anything (),
"The file name of the lateral viscosity prefactor. ");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"The file name of the lateral viscosity prefactor. ");
"The file name of the lateral viscosity prefactor.");

Patterns::List(Patterns::Double (0.)),
"A list of depth values that indicate where the transitions between "
"the different conductivity parameter sets should occur in the "
"'p-T-dependent' Thermal conductivity formulation (in most cases, "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"'p-T-dependent' Thermal conductivity formulation (in most cases, "
"'p-T-dependent' thermal conductivity formulation (in most cases, "

prm.declare_entry ("Reference thermal conductivities", "2.47, 3.81, 3.52, 4.9",
Patterns::List(Patterns::Double (0.)),
"A list of base values of the thermal conductivity for each of the "
"horizontal layers in the 'p-T-dependent' Thermal conductivity "
Copy link
Member

Choose a reason for hiding this comment

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

Here and all the other parameter descriptions.

Suggested change
"horizontal layers in the 'p-T-dependent' Thermal conductivity "
"horizontal layers in the 'p-T-dependent' thermal conductivity "

@@ -209,11 +422,47 @@ namespace aspect
{
data_directory = Utilities::expand_ASPECT_SOURCE_DIR(prm.get ("Data directory"));
material_file_name = prm.get ("Material file name");
lateral_viscosity_prefactor = prm.get_double ("Lateral viscosity prefactor");
lateral_viscosity_prefactor_file_name = prm.get ("Lateral viscosity prefactor file name");
//lateral_viscosity_prefactor = prm.get_double ("Lateral viscosity prefactor");
Copy link
Member

Choose a reason for hiding this comment

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

remove commented line

@RanpengLi
Copy link
Contributor Author

Thank you! I have addressed all the comments and added a changelog.

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.

Much closer, just a few more comments.

const double p_dependence = reference_thermal_conductivities[layer_index] + conductivity_pressure_dependencies[layer_index] * pressure;

// Make reasonably sure we will not compute any invalid values due to the temperature-dependence.
// Since both the temperature-dependence and the saturation term scale with (Tref/T), we have to
Copy link
Member

Choose a reason for hiding this comment

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

time scalehas not been addressed

Changed: The entropy plugin now includes a plasticity rheology, which can be set by cohesion and frictional angle. It can also use a viscosity prefactor profile, set the lateral viscosity variation limit, and choose p-T dependent conductivity

<br>
(Ranpeng Li, 2023/07/12)
Copy link
Member

Choose a reason for hiding this comment

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

this file is currently missing a newline at the end of the file. This is an annoying default property of VS code, you can check here how to change the behavior: https://stackoverflow.com/questions/44704968/visual-studio-code-insert-newline-at-the-end-of-files

Changed: The entropy plugin now includes a plasticity rheology, which can be set by cohesion and frictional angle. It can also use a viscosity prefactor profile, set the lateral viscosity variation limit, and choose p-T dependent conductivity

<br>
(Ranpeng Li, 2023/07/12)
Copy link
Member

Choose a reason for hiding this comment

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

You probably want to include Juliane as author here as well, or did you do all of these changes on your own?

@RanpengLi
Copy link
Contributor Author

I just fixed them. Sorry!

@gassmoeller
Copy link
Member

Thanks for addressing the comments. Ready to merge when the testers are done.

@gassmoeller
Copy link
Member

I think you will have to update the test results (or something changed in the test results)

@gassmoeller
Copy link
Member

/rebuild

@gassmoeller
Copy link
Member

It looks like the linux/tests tester also failed, so likely something changed on the main branch. Just update the test results again with the reference tester results and it should pass.

@gassmoeller gassmoeller merged commit 1f1b1f5 into geodynamics:main Jul 13, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants