Add Tian Parametrization to Reactive Fluid Transport #5313

danieldouglas92 · 2023-07-14T04:58:25Z

Implement a solid-fluid reaction scheme that uses parametrized phase diagrams from Tian et al., 2019 https://doi.org/10.1029/2019GC008488.

A comparison between the peridotite phase diagram defined in the study and the ASPECT reproduction are shown below. Note that the y-axis is inverted in the ASPECT models since pressure increases with depth. The low-res output shows what is implemented as a test, and the high-res output is shown for better comparison to the study.

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

Describe what you did in this PR and why you did it.

Before your first pull request:

I have read the guidelines in our CONTRIBUTING.md document.

For all pull requests:

I have followed the instructions for indenting my code.

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.

naliboff

@danieldouglas92 - Thanks for the contribution and at first glance the PR is in good shape. I made comments on the portions specific to the Tian et al. implementation. For now, a number of comments about documentation. Let's discuss how to proceed after #5245 is ready to be merged.

source/material_model/reactive_fluid_transport.cc

tests/tian_approximation.prm

naliboff

@danieldouglas92 - Thanks for continuing the work on this and including all of the tests! Overall, I think the structure of the code is in good shape and the PR is not far off from being after some moderate revisions.

I have some initial comments throughout, but one thing that stood out is that some of the code in your commits is not related to the Tian approximation and may have been introduced during a rebase?

doc/modules/changes/20230714_danieldouglas92

naliboff · 2024-02-23T22:02:58Z

include/aspect/material_model/reactive_fluid_transport.h

@@ -137,6 +141,10 @@ namespace aspect

        // Time scale for fluid release and absorption.
        double fluid_reaction_time_scale;
+        double max_peridotite_water;


Suggested change

double max_peridotite_water;

// Parameters for Tian et al. 2018 (tian_approximation) reactive fluid transport scheme

double max_peridotite_water;

I wonder if we should rename these variables to something like tian_max_peridotite water to avoid confusion with other schemes that involve volatiles, or perhaps these variables are general enough to be reused for other schemes?

I suspect that at least for now they're specific to the Tian scheme

naliboff · 2024-02-23T22:05:45Z

source/material_model/reactive_fluid_transport.cc

-                // at any point (stored in the melt_fractions vector) is
-                // equal to the sum of the bound fluid content and porosity.
-                const unsigned int bound_fluid_idx = this->introspection().compositional_index_for_name("bound_fluid");
+                // A fluid-rock reaction model where no reactions occur.


Is this part of this PR, or was it included as part of a rebase?

Ahh I must have made a mistake resolving the merge conflicts! good catch

source/material_model/reactive_fluid_transport.cc

naliboff · 2024-02-23T22:34:35Z

include/aspect/material_model/reactive_fluid_transport.h

@@ -137,6 +141,10 @@ namespace aspect

        // Time scale for fluid release and absorption.
        double fluid_reaction_time_scale;
+        double max_peridotite_water;
+        double max_gabbro_water;


Look farther ahead, it looks like this and the other variables below are currently not being used to enforce upper limits? Is the plan to add them in a forthcoming commit?

These are used in the function tian_equilibrium_bound_water_content() to limit the bound water for the respective rock type.

source/material_model/reactive_fluid_transport.cc

naliboff · 2024-02-23T22:36:40Z

source/material_model/reactive_fluid_transport.cc

@@ -246,12 +356,26 @@ namespace aspect
                             "computed. If the model does not use operator splitting, this parameter is not used. "
                             "Units: yr or s, depending on the ``Use years "
                             "in output instead of seconds'' parameter.");
+          prm.declare_entry ("Maximum weight percent sediment", "3",


Is the plan to automatically convert these values to a true fraction (i.e., 0.03) down the line?

Yes this is already done in the function tian_equilibrium_bound_water_content, I figured it was more intuitive to input weight % as a % instead of a fraction, but I'm not attached to it if you think a fraction is a better convention

naliboff

@danieldouglas92 - Thanks for the updates and this is getting much closer. I added another round of comments, and after that will probably only need to look at it once more (@jdannberg - can you also take a look at some point)? Thanks!

doc/modules/changes/20230714_danieldouglas92

source/material_model/reactive_fluid_transport.cc

naliboff · 2024-02-24T22:20:04Z

source/material_model/reactive_fluid_transport.cc

-        {
-          AssertThrow(this->get_material_model().is_compressible() == false,
-                      ExcMessage("The Fluid-reaction scheme zero solubility must be used with an incompressible base model."));
-        }


Why is this removed?

That was a mistake! Thanks for catching that

tests/tian_MORB.prm

tests/tian_peridotite.prm

naliboff · 2024-02-24T22:23:27Z

tests/tian_peridotite.prm

+
+# Post processing
+subsection Postprocess
+  set List of postprocessors = visualization, composition statistics, velocity statistics


The visualization files are included in this PR, but would be great to have so the data to reproduce the figures can be quickly plotted without running the models. Also, I would output in ascii rather than vtu format.

I actually didn't mean to include the visualization postprocessor, if you think it would be worth it I can output it to ASCII but I don't think it's necessary for the tests to be effective so I can just remove it

tests/tian_peridotite.prm

naliboff · 2024-02-26T17:39:00Z

@danieldouglas92 - To follow up on a comment from my last review, I think it would be really useful to include some figures in the discussion to show how running the tests at higher resolutions can be used to reproduce the figures in the Tian paper. You could also include the python script showing how to make the plots, and then later add it is as part of a new cookbook.

jdannberg

Thank you for adding this to ASPECT. I've made some suggestions, but otherwise this looks good to me!

include/aspect/material_model/reactive_fluid_transport.h

source/material_model/reactive_fluid_transport.cc

tests/tian_peridotite.prm

jdannberg

Looks good to me (other than the two typos)!

include/aspect/material_model/reactive_fluid_transport.h

jdannberg · 2024-02-26T22:34:38Z

But I agree with @naliboff, it would be great to include an output of one of the tests in the discussion in this pull request, just so that we have a documentation of how it looks like in relation to the figures from the Tian model.

danieldouglas92 · 2024-02-27T01:24:36Z

But I agree with @naliboff, it would be great to include an output of one of the tests in the discussion in this pull request, just so that we have a documentation of how it looks like in relation to the figures from the Tian model.

Updated it in the initial PR message!

…al., 2018

danieldouglas92 force-pushed the tian_parametrization branch from 098d075 to 0cc3093 Compare July 14, 2023 17:56

danieldouglas92 changed the title ~~Add Tian Parametrization to Reactive Fluid Transport [WIP]~~ Add Tian Parametrization to Reactive Fluid Transport Jul 14, 2023

naliboff reviewed Jul 14, 2023

View reviewed changes

danieldouglas92 force-pushed the tian_parametrization branch from f1dceaa to fd724b7 Compare July 14, 2023 23:53

anne-glerum added this to In Progress in Fluids and Melts Feb 23, 2024

danieldouglas92 force-pushed the tian_parametrization branch 2 times, most recently from a1c8993 to e0ac649 Compare February 23, 2024 20:42

naliboff requested changes Feb 23, 2024

View reviewed changes

danieldouglas92 force-pushed the tian_parametrization branch from 7480212 to 90710ba Compare February 24, 2024 17:12

naliboff reviewed Feb 24, 2024

View reviewed changes

danieldouglas92 force-pushed the tian_parametrization branch from 90710ba to d04bdca Compare February 25, 2024 03:56

naliboff added the ready to test label Feb 26, 2024

jdannberg reviewed Feb 26, 2024

View reviewed changes

jdannberg approved these changes Feb 26, 2024

View reviewed changes

include/aspect/material_model/reactive_fluid_transport.h Outdated Show resolved Hide resolved

include/aspect/material_model/reactive_fluid_transport.h Outdated Show resolved Hide resolved

danieldouglas92 added 4 commits February 26, 2024 20:26

Add a parametrization for 4 different rock compositions from Tian et …

f15cbb4

…al., 2018

Rebase to main

d21f3cf

Add tests for tian approximation

fa76f8b

Address comments

e4f6055

danieldouglas92 force-pushed the tian_parametrization branch from 6b75ac9 to e4f6055 Compare February 27, 2024 01:26

naliboff approved these changes Feb 27, 2024

View reviewed changes

naliboff merged commit 0d29d13 into geodynamics:main Feb 27, 2024
7 checks passed

Fluids and Melts automation moved this from In Progress to Done Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tian Parametrization to Reactive Fluid Transport #5313

Add Tian Parametrization to Reactive Fluid Transport #5313

danieldouglas92 commented Jul 14, 2023 •

edited

naliboff left a comment

naliboff left a comment

naliboff Feb 23, 2024

naliboff Feb 23, 2024

danieldouglas92 Feb 24, 2024

naliboff Feb 23, 2024

danieldouglas92 Feb 24, 2024

naliboff Feb 23, 2024

danieldouglas92 Feb 24, 2024

naliboff Feb 23, 2024

danieldouglas92 Feb 24, 2024 •

edited

naliboff left a comment

naliboff Feb 24, 2024

danieldouglas92 Feb 25, 2024 •

edited

naliboff Feb 24, 2024

danieldouglas92 Feb 25, 2024 •

edited

naliboff commented Feb 26, 2024

jdannberg left a comment

jdannberg left a comment

jdannberg commented Feb 26, 2024

danieldouglas92 commented Feb 27, 2024

Add Tian Parametrization to Reactive Fluid Transport #5313

Add Tian Parametrization to Reactive Fluid Transport #5313

Conversation

danieldouglas92 commented Jul 14, 2023 • edited

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

naliboff left a comment

Choose a reason for hiding this comment

naliboff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldouglas92 Feb 24, 2024 • edited

Choose a reason for hiding this comment

naliboff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldouglas92 Feb 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldouglas92 Feb 25, 2024 • edited

Choose a reason for hiding this comment

naliboff commented Feb 26, 2024

jdannberg left a comment

Choose a reason for hiding this comment

jdannberg left a comment

Choose a reason for hiding this comment

jdannberg commented Feb 26, 2024

danieldouglas92 commented Feb 27, 2024

danieldouglas92 commented Jul 14, 2023 •

edited

danieldouglas92 Feb 24, 2024 •

edited

danieldouglas92 Feb 25, 2024 •

edited

danieldouglas92 Feb 25, 2024 •

edited