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

Cookbook for mantle convection model using tomography and other geophysical data. #5256

Merged
merged 11 commits into from Oct 8, 2023

Conversation

alarshi
Copy link
Contributor

@alarshi alarshi commented Jul 10, 2023

This PR is a follow up from #PR 4727, which adds a cookbook to setup a mantle convection model using tomography and other geophysical data. I addressed most of the comments left on #PR 4727, but need help if the cookbook is clear enough and makes sense for a 2D spherical shell model. Thank you!

Before your first pull request:

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.

@alarshi alarshi force-pushed the material_model_branch branch 3 times, most recently from bdece88 to 288b84a Compare July 12, 2023 23:07
@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.

Just some first comments to let you get started, I still need to look into a lot of the files.

cookbooks/equilibrium_grain_size/doc/Fig_composition.png Outdated Show resolved Hide resolved
cookbooks/equilibrium_grain_size/plugins/CMakeLists.txt Outdated Show resolved Hide resolved
@@ -106,5 +106,6 @@ cookbooks/global_melt/doc/global_melt.md
cookbooks/mid_ocean_ridge/doc/mid_ocean_ridge.md
cookbooks/kinematically_driven_subduction_2d/doc/kinematically_driven_subduction_2d.md
cookbooks/inclusions/doc/inclusions.md
cookbooks/equilibrium_grain_size/doc/tomography_based_model.md
Copy link
Member

Choose a reason for hiding this comment

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

Here you name the section 'tomography based model', but the folder is called 'equilibrium grain size'. I think both are not very good names for this cookbook (tomography based model is quite unspecific, and the model has not much to do with grain size anymore). I would suggest: "Tomography based plate motions" or "tomography_based_plate_motions.prm. I know this is a lot of work, but please rename the folder and the file, and all references to the old names in all files (search and replace is your friend :-)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for suggestion!

@alarshi
Copy link
Contributor Author

alarshi commented Jul 13, 2023

Thank you @gassmoeller for the review! I addressed your comments in the latest commits. We now have a different name for the model and I have added the changelog entry related to this PR.

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.

Some more comments. Still havent gotten to the main files.

@bangerth bangerth changed the title Material model branch Cookbook for mantle convection model using tomography and other geophysical data. Sep 29, 2023
@bangerth
Copy link
Contributor

bangerth commented Oct 6, 2023

There are two warnings from sphinx:

/home/docs/checkouts/readthedocs.org/user_builds/aspect-documentation/checkouts/5256/doc/sphinx/user/cookbooks/cookbooks/tomography_based_plate_motions/doc/tomography_based_plate_motions.md:51: WARNING: Include file '/home/docs/checkouts/readthedocs.org/user_builds/aspect-documentation/checkouts/5256/doc/sphinx/user/cookbooks/cookbooks/tomography_based_plate_motions/doc/tomography_based_model.part.prm' not found or reading it failed
/home/docs/checkouts/readthedocs.org/user_builds/aspect-documentation/checkouts/5256/doc/sphinx/user/cookbooks/geophysical-setups.md:93: WARNING: toctree contains reference to nonexisting document 'user/cookbooks/<<<<<<< HEAD'

Comment on lines 121 to 122
# We use intial temperatures from an ASCII file but we also use adiabatic
# boundary plugin such it returns very small temperatures, and on taking
Copy link
Contributor

Choose a reason for hiding this comment

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

Something is grammatically not quite right here.

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 rephrased what I wanted to say, I hope it makes more sense now.

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

I have not looked in detail through the material model implementation, but assume that it is largely copied and pasted from other material models.

I think I've looked through everything else, though.

doc/sphinx/user/cookbooks/geophysical-setups.md Outdated Show resolved Hide resolved
Comment on lines +94 to +101
double uppermost_mantle_thickness = 0.0;
if (Plugins::plugin_type_matches<const MaterialModel::TomographyBasedPlateMotions<dim>>(this->get_material_model()))
{
const MaterialModel::TomographyBasedPlateMotions<dim> &material_model
= Plugins::get_plugin_as_type<const MaterialModel::TomographyBasedPlateMotions<dim>> (this->get_material_model());

uppermost_mantle_thickness = material_model.get_depth_of_base_of_uppermost_mantle();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You initialize uppermost_mantle_thickness to zero if that plugin is not found. Is that the correct value? Or are you using the 0.0 value to express that the value is uninitialized and hope that you never use the value below? It seems to me the former, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it it the former case. If we use densities from the material model (in this case, tomography_based_plate_motions) , we will simply use the densities from the material model until the uppermost_mantle_thickness depth. We initialize it to zero so that by default the density profile is from the input reference density profile.

}
else
{
// We only want to use the PREM densities in the part of the model that also
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a copy-paste error? I thought you're not using PREM, or did I midunderstand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, we are using PREM for defining our reference profile against the tomography. But, "reference densities" may be more appropriate.

Comment on lines +155 to +175
// The update() function updates the profile that stores the laterally averaged viscosity.
// This is needed to compute the viscosity in the material model (since the viscosities are rescaled
// so that the lateral average matches the reference profile). Since the viscosity depends on both
// temperature and velocity, we want to update the lateral average profile after each temperature
// and Stokes solve.
this->get_signals().post_stokes_solver.connect([&](const SimulatorAccess<dim> &,
const unsigned int ,
const unsigned int ,
const SolverControl &,
const SolverControl &)
{
this->update();
});

this->get_signals().post_advection_solver.connect([&](const SimulatorAccess<dim> &,
const unsigned int ,
const unsigned int ,
const SolverControl &)
{
this->update();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand it right, your model is instantaneous and so you never actually solve for the temperature. In that case, the comment is not quite right, nor is it necessary to connect to the post_advection_solver.

This is mostly for clarification to myself: You could update the comment and remove the second connect() call. But perhaps it is best to leave it in place anyway just in case someone wanted to extend our model to the time-dependent case?

@alarshi
Copy link
Contributor Author

alarshi commented Oct 7, 2023

Thank you for the review! I have addressed all your comments, except one, in the latest commits. I will discuss the remaining one with you tomorrow.

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.

Just a few typos. Afterwards ready to merge.

@alarshi
Copy link
Contributor Author

alarshi commented Oct 7, 2023

Thank you for the review! I addressed your comments in the latest commit and added the plugin in the tests/ directory. Hopefully, the test works now :)

Copy link
Member

Choose a reason for hiding this comment

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

The tester is failing, because this .prm file is not in the base directory of the cookbook, but in the models/ subdirectory. You can either move the .prm file there, or create a custom rule for this cookbook in cookbooks/check.mk. I would prefer moving the file into the base folder of the cookbook (also check if you have to adjust the test .prm for that.

gassmoeller and others added 11 commits October 7, 2023 20:01
Add test data files;

implement equilibrium grain size

Add scripts from Dannberg et al, 2017

Cleanup and add main()

Add compile command to repo

Temporarily store reference result from python script for comparison with c++

Add constant grain size model

model: remove nullspace

add matrix-free.prm

Make constant grain size models converge. Improve setup.

fix equilibrium grain size and add a test file

Add prem profile

Becker viscosity model.

parent f1fd292
author Arushi Saxena <asaxena1@login01.expanse.sdsc.edu> 1622820847 -0700
committer asaxena <saxena.arushi@ufl.edu> 1653082535 -0400

parent f1fd292
author Arushi Saxena <asaxena1@login01.expanse.sdsc.edu> 1622820847 -0700
committer asaxena <saxena.arushi@ufl.edu> 1653082518 -0400

parent f1fd292
author Arushi Saxena <asaxena1@login01.expanse.sdsc.edu> 1622820847 -0700
committer asaxena <saxena.arushi@ufl.edu> 1653082460 -0400

Added no surface rotation in nullspace.

Moved files according to aspect data structure.

Removed unnecessary files.

fix a file while rebase.

remove unnecessary files.

Indentation fix.

Addressed initial suggestions from the PR.

Changes to remove reference profile in adiabatic conditions.

Fix for using adiabatic conditions.

Addressed most comments on the souce files.

Fixed the unit style.

Remove adiabatic conditions from reference profile.

Changes with recent ASPECT version.

Fix seg fault from composition manager.

Parameter file and plugin changes consistent with recent ASPECT.

Remaining suggestions from PR.

Modifications in the initial temperature pointer, consistent with current main.

fix indent.

Add references and check if the cookbook documentation works.

Indent changes.

Add figures and documentation related files.

Add the reference file.

Fix a bib entry.

Fix formatting of headings and small doc changes.

Fix the path to a file.
Addressed remaining suggestions from the PR.
Address some comments in the PR.

Address remaining suggestions.
Addressed comments from the PR.

fix indent
Address comments.
Fix the test output.
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, good to go when the testers are done.

@gassmoeller
Copy link
Member

The relevant testers passed, so let's get this merged. Congrats @alarshi!

@gassmoeller gassmoeller merged commit 4024c3d into geodynamics:main Oct 8, 2023
6 of 7 checks passed
@bangerth
Copy link
Contributor

bangerth commented Oct 8, 2023

Yay, very nice!

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