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

add a new benchmark for volatiles release, migration and reabsoption #5214

Merged
merged 4 commits into from Jul 9, 2023

Conversation

jdannberg
Copy link
Contributor

This PR adds a new benchmark for water solubility. The new plugin in the benchmark folder can also serve as an example for how water solubility can be implemented in a plugin in ASPECT.

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.

Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

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

@jdannberg - Thank you for this contribution, which will be really useful for a wide range of groups!

There are lots of comments, but nearly all related to parameter naming/descriptions and related documentation. In short, they all boil down to how generic to make the plugin at this stage, as well as in the future. Aside from these conventions, the code and example are in good shape, and can be merged with little modification.

Here are the general questions I have in mind:

  1. Should this plugin be added in the material model section (rather than as a cookbook), or should that step occur later down the line? I think this should occur sooner rather than later for testing purposes, but it does not need to be now.

  2. As the model can in theory be expanded to deal with any type of geofluid, should the name be changed to something like reactive fluid transport or reactive fluids? My preference is for the former, as it indicates the material both deals with the reaction and advection of fluids.

  3. Where possible, I think variable names containing melt or volatiles should be reduced to more generic names (fluids), unless they specifically are addressing a specific type of geofluid.

  4. I left one comment/question about how the material model could be modified to use either the two-phase flow equations or the simpler darcy field method for advecting fluids.

To reiterate, I am fine if many of the suggested changes above occur when the code (or a copy of it) is moved to the material model section.

@danieldouglas92 and Ryan - what are your thoughts?

Thanks Juliane!

in minerals, or it can be present as a fluid phase that can migrate relative to the
solid rock. The fraction of this free water then represents the porosity of the
material. Therefore, the terms describing melting and freezing of a silicate melt
in the equations would instead describe the release and reabsorption of water into
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
in the equations would instead describe the release and reabsorption of water into
in the equations would instead describe the release and reabsorption of water (or other volatiles) into

@@ -0,0 +1,409 @@
/*
Copyright (C) 2014 - 2020 by the authors of the ASPECT code.
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
Copyright (C) 2014 - 2020 by the authors of the ASPECT code.
Copyright (C) 2023 - by the authors of the ASPECT code.

using namespace dealii;

/**
* Volatiles material model.
Copy link
Contributor

Choose a reason for hiding this comment

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

In principle, this material could also be used for simulating reactive melt transport. As such, I wonder if a more generic name for the classes (say Fluids) would be a good idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I envisioned this is that what I wrote here is really only the benchmark. If @danieldouglas92 wants to implement a more complex model, I thought he would just make a copy of this, because I was assuming that the two of you might want to change a lot of things and this is really just a minimal working example.

You could then pick a more general name for your material model.

*/
std::unique_ptr<MaterialModel::Interface<dim>> base_model;

double reference_rho_f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some point it would be good to add documentation for these variables. @danieldouglas92 and I can also do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am adding a short description, but these are also all documented in the declare parameters function (since they are read from the input file).

double eta_f;
double reference_permeability;
double alpha_phi;
double melt_compressibility;
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
double melt_compressibility;
double fluid_compressibility;

Better to use a more generic name here?

template <int dim>
void
Volatiles<dim>::
melt_fractions (const MaterialModel::MaterialModelInputs<dim> &in,
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
melt_fractions (const MaterialModel::MaterialModelInputs<dim> &in,
fluid_fractions (const MaterialModel::MaterialModelInputs<dim> &in,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, this is not something I can change, this is defined by the interface.

{
namespace MaterialModel
{
ASPECT_REGISTER_MATERIAL_MODEL(Volatiles,
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
ASPECT_REGISTER_MATERIAL_MODEL(Volatiles,
ASPECT_REGISTER_MATERIAL_MODEL(Reactive Fluid Transport,

Copy link
Contributor

Choose a reason for hiding this comment

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

Another name could be Reactive Fluids

namespace MaterialModel
{
ASPECT_REGISTER_MATERIAL_MODEL(Volatiles,
"volatiles",
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
"volatiles",
"reactive fluid transport",

{
ASPECT_REGISTER_MATERIAL_MODEL(Volatiles,
"volatiles",
"Material model that can advect volatiles and contains "
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
"Material model that can advect volatiles and contains "
"A composited material model composited designed to both advect "
"fluids and calculate equilibrium fluid fractions. It currently contains "

// Fill the melt outputs if they exist.
MeltOutputs<dim> *melt_out = out.template get_additional_output<MeltOutputs<dim>>();

if (melt_out != nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this section of code be expanded to have a condition where the MeltOutputs are not filled, and instead the fluids are advected using the single-phase Darcy method?

As an alternative, could a trick be done where the porosity given to the MeltOutputs is set to 0, such that is solves the normal equations for solid deformation and the fluids are advected with the darcy field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you could just switch off melt transport and then change the porosity to be of type darcy_field. I do not think this would require changing the code, but there may be some things I am not considering at the moment.

@ryanstoner1
Copy link
Contributor

ryanstoner1 commented Jul 8, 2023

@naliboff
A couple thoughts on the questions. I think making the syntax more general would allow wider interest from the community.

1. Should this plugin be added in the material model section (rather than as a cookbook), or should that step occur later down the line? I think this should occur sooner rather than later for testing purposes, but it does not need to be now.

If adding this to the material model would help with numerical stability then the material model might be good to do sooner. If not then my vote is for whatever makes things the most modular.

2. As the model can in theory be expanded to  deal with any type of geofluid, should the name be changed to something like `reactive fluid transport` or `reactive fluids`? My preference is for the former, as it indicates the material both deals with the reaction and advection of fluids.

A vote for reactive fluid transport for the same reasons.

3. Where possible, I think variable names containing melt or volatiles should be reduced to more generic names (fluids), unless they specifically are addressing a specific type of geofluid.

This would be more correct from a geochemistry perspective.

@jdannberg
Copy link
Contributor Author

@naliboff Thank you for your review! I addressed your comments. For your more general questions:

  1. Should this plugin be added in the material model section (rather than as a cookbook), or should that step occur later down the line? I think this should occur sooner rather than later for testing purposes, but it does not need to be now.

I would leave the current version as a benchmark, but put the new model @danieldouglas92 is making into the material model folder. This way, this simple model can serve as an example for how the implementation of such a model would work and illustrate the general concept, and the model Daniel is writing (that is actually applicable to Earth) can be used more easily by others.

Also, tests can include files from the benchmarks folder (as I did in this PR) so I do not think it would make a difference for that.

  1. As the model can in theory be expanded to deal with any type of geofluid, should the name be changed to something like reactive fluid transport or reactive fluids? My preference is for the former, as it indicates the material both deals with the reaction and advection of fluids.
  1. Where possible, I think variable names containing melt or volatiles should be reduced to more generic names (fluids), unless they specifically are addressing a specific type of geofluid.

Same here, my model is just for water, so everywhere where it said melt, I changed that, but where I referred to water and water content, I left it like that. When you make a new model that is more general, that model could get a different name.

Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

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

@jdannberg - Thanks for all of the changes, and agree the best path forward is to implement the other changes discussed (move to plugin to material model section, adjust name of material model) in the upcoming PR from @danieldouglas92.

@naliboff
Copy link
Contributor

naliboff commented Jul 9, 2023

@jdannberg - One of the the testers had an error while attempting to run the solubility cookbook test (error from "console" below):


Exception 'ExcMessage (std::string("Could not successfully load shared library <") + filename + ">. The operating system reports " + "that the error is this: <" + dlerror() + ">.")' on rank 0 on processing:


An error occurred in line <288> of file </jenkins/workspace/aspect_PR-5214/source/main.cc> in function
void possibly_load_shared_libs(const string&)
The violated condition was:
handle != nullptr
Additional information:
Could not successfully load shared library
<./plugin/libsolubility.debug.so>. The operating system reports that
the error is this: <./plugin/libsolubility.debug.so: cannot open
shared object file: No such file or directory>.

Would a fix be to specify the full path of the plugin in the benchmark .prm file?
set Additional shared libraries = $ASPECT_SOURCE_DIR/benchmarks/solubility/plugin/libsolubility.so

@naliboff
Copy link
Contributor

naliboff commented Jul 9, 2023

@jdannberg - thanks for the fix. Ready to merge after resolving the merge conflict with the change log.

@jdannberg
Copy link
Contributor Author

Okay! Fixed the changelog as well.

@naliboff naliboff merged commit f260215 into geodynamics:main Jul 9, 2023
6 checks passed
@jdannberg jdannberg deleted the volatiles branch July 9, 2023 18:54
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