-
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
Use conductivity interface to remove code duplication #6215
Use conductivity interface to remove code duplication #6215
Conversation
3105fc9
to
71a64c0
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.
Are you still working on this? I went though the PR, but I think while I did, you removed the constant plugin...
Anyway, this looks good to me, the only question I had was:
In all other plugin systems (EOS, viscosity, reactions) we compute the property for separately for each point. Here you decided to compute the conductivity for all points in the material model inputs. Why did you think that was the right choice?
@@ -118,7 +119,8 @@ namespace aspect | |||
* The viscosity of this model is based on the paper | |||
* Steinberger & Calderwood 2006: "Models of large-scale viscous flow in the | |||
* Earth's mantle with constraints from mineral physics and surface | |||
* observations". The thermal conductivity is constant and the other | |||
* observations". The thermal conductivity is constant or following a |
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.
* observations". The thermal conductivity is constant or following a | |
* observations". The thermal conductivity is constant or follows a |
source/material_model/simpler.cc
Outdated
@@ -44,6 +44,8 @@ namespace aspect | |||
// The Simpler model does not depend on composition | |||
EquationOfStateOutputs<dim> eos_outputs (1); | |||
|
|||
thermal_conductivity.evaluate(in,out); |
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 question: The viscosity and eos plugins (and the reaction models as well) compute their properties inside the loop over all points, whereas your conductivity is outside and makes its own loop. Is this on purpose?
This of course has advantages and disadvantages of what one can do with it.
include/aspect/coordinate_systems.h
Outdated
@@ -25,6 +25,8 @@ namespace aspect | |||
{ | |||
namespace Utilities | |||
{ | |||
using namespace dealii::Utilities; |
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.
Is that related?
using namespace dealii; | ||
|
||
/** | ||
* A class that implements a pressure and temperature-dependent thermal conductivity |
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 class that implements a pressure and temperature-dependent thermal conductivity | |
* A class that implements a pressure- and temperature-dependent thermal conductivity |
|
||
private: | ||
/** | ||
* Parameters for the temperature- and pressure dependence of the |
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.
* Parameters for the temperature- and pressure dependence of the | |
* Parameters for the temperature and pressure dependence of the |
cc4bd3b
to
a2df759
Compare
a2df759
to
f6f3655
Compare
Thanks for the review, I have addressed all the minor comments.
Yes, you are right that this is handled differently than for the equation of state and the rheology plugins. I am still unsure about the best path forward here, but I think in retrospect the decision to let equation of state and rheology compute properties for one point at a time was a mistake. The current interfaces to the EoS and rheology plugins often look like this:
where Anyway, we already use the interface I propose here for the reaction model (in We should probably have a discussion about which interface is more useful in the long run. Is it ok to move forward with this PR in the meantime (given that #6211 has already introduced the inferface and this PR just adds one more plugin)? |
This PR is an example of what will be possible when #6211 is merged. Currently the material models
Steinberger
andEntropy
use the same thermal conductivity model, either constant or a temperature-pressure dependent approximation. In order to do that they duplicate the code incl. declaration and parsing of parameters. In this PR I outsourced all of that to the new thermal conductivity plugins and either use the pluginConstant
orTosiStackhouse
depending on user input. Ultimately I want to get to the state where the thermal conductivity plugins form their own plugin system, where the user can select from the input file which plugin to use (ideally for most or all material models).