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

Use conductivity interface to remove code duplication #6215

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

gassmoeller
Copy link
Member

This PR is an example of what will be possible when #6211 is merged. Currently the material models Steinberger and Entropy 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 plugin Constant or TosiStackhouse 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).

Copy link
Contributor

@jdannberg jdannberg left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* observations". The thermal conductivity is constant or following a
* observations". The thermal conductivity is constant or follows a

@@ -44,6 +44,8 @@ namespace aspect
// The Simpler model does not depend on composition
EquationOfStateOutputs<dim> eos_outputs (1);

thermal_conductivity.evaluate(in,out);
Copy link
Contributor

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.

@@ -25,6 +25,8 @@ namespace aspect
{
namespace Utilities
{
using namespace dealii::Utilities;
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Parameters for the temperature- and pressure dependence of the
* Parameters for the temperature and pressure dependence of the

@gassmoeller gassmoeller force-pushed the use_conductivity_interface branch 2 times, most recently from cc4bd3b to a2df759 Compare February 3, 2025 14:14
@gassmoeller gassmoeller force-pushed the use_conductivity_interface branch from a2df759 to f6f3655 Compare February 3, 2025 14:53
@gassmoeller
Copy link
Member Author

Thanks for the review, I have addressed all the minor comments.

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.

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:

      ViscoPlastic<dim>::
      calculate_isostrain_viscosities (const MaterialModel::MaterialModelInputs<dim> &in,
                                       const unsigned int i,
...

where in contains inputs for all points, and i determines which point should be computed at the moment. This is awkward, if input for all points is handed over, why not immediately compute all points? Our reasoning back then (I think) was that we wanted to keep the phase functions and the task of averaging the different phases out of the individual plugins, and let the material model handle this, but I dont recall how this is exactly connected.

Anyway, we already use the interface I propose here for the reaction model (in grain_size_evolution.cc and katz_2003_mantle_melting.cc), so both types of interfaces already exist.

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)?

@jdannberg jdannberg merged commit ae4290f into geodynamics:main Feb 4, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants