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.

Sign up for GitHub

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

Jump to bottom

Make the GMG solver the new default solver. #5211

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Make the GMG solver the new default solver. #5211

merged 1 commit into from
Jun 13, 2024

Conversation

gassmoeller
Copy link
Member

For now this is just a trial to see which tests break.

@bangerth
Copy link
Contributor

bangerth commented Jul 8, 2023

Get out the popcorn!

@bangerth
Copy link
Contributor

bangerth commented Jul 8, 2023

918 of 1025 tests are failures -- not bad, you broke ~90% of all tests :-)

@bangerth
Copy link
Contributor

bangerth commented Jul 8, 2023

And the test output diff is 100 MB!

@gassmoeller gassmoeller mentioned this pull request Jul 9, 2023
Merged
@tjhei tjhei mentioned this pull request Feb 8, 2024
Open
6 tasks
@gassmoeller gassmoeller force-pushed the make_gmg_default branch 2 times, most recently from 1c3905f to fa5d97a Compare May 31, 2024 16:48
@gassmoeller
Copy link
Member Author

/rebuild

@gassmoeller gassmoeller changed the title [WIP] Make the GMG solver the new default solver. Make the GMG solver the new default solver. May 31, 2024
@gassmoeller
Copy link
Member Author

I think this is now ready for a first review. I still want to look through the test output one more time, but I would appreciate feedback on the way I implemented the changes and the fallback functionality in the code.

tjhei
tjhei reviewed Jun 1, 2024
View reviewed changes
source/simulator/helper_functions.cc Outdated

pcout << "Warning, the GMG preconditioner is not supported for models locally conservative discretization. Disabling GMG preconditioner." << std::endl;
}
else if (geometry_model->get_periodic_boundary_pairs().size() > 0)
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer true:v
https://github.com/geodynamics/aspect/blob/b2a9312199df918853086989fed6f4d2fb63b199/source/simulator/stokes_matrix_free.cc#L2389C1-L2410C6

gassmoeller
gassmoeller commented Jun 1, 2024
View reviewed changes
source/simulator/parameters.cc Outdated
@@ -1327,7 +1327,7 @@ namespace aspect
// preconditioner
prm.enter_subsection ("Material model");
{
prm.declare_entry ("Material averaging", "none",
prm.declare_entry ("Material averaging", "project to Q1 only viscosity",
Copy link
Member Author

Choose a reason for hiding this comment

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

I have to think about if this is the right thing to do, because it silently changes the averaging even for models that use AMG preconditioner. I also have to check the cookbooks that discuss averaging to make sure they do use the correct averaging and do not contradict this new default value.

@gassmoeller gassmoeller force-pushed the make_gmg_default branch 6 times, most recently from b113086 to a145b93 Compare June 4, 2024 17:39
@gassmoeller gassmoeller force-pushed the make_gmg_default branch 6 times, most recently from 60bda51 to b2b2608 Compare June 10, 2024 17:30
tjhei
tjhei reviewed Jun 10, 2024
View reviewed changes
include/aspect/material_model/interface.h Outdated
@@ -712,7 +712,8 @@ namespace aspect
log_average,
harmonic_average_only_viscosity,
geometric_average_only_viscosity,
project_to_Q1_only_viscosity
project_to_Q1_only_viscosity,
solver_default
Copy link
Member

Choose a reason for hiding this comment

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

default_averaging?

@gassmoeller gassmoeller force-pushed the make_gmg_default branch 3 times, most recently from 521ca3e to 7407075 Compare June 10, 2024 20:31
@gassmoeller gassmoeller mentioned this pull request Jun 10, 2024
Merged
@gassmoeller gassmoeller force-pushed the make_gmg_default branch 2 times, most recently from db68452 to dd46a7a Compare June 11, 2024 06:22
@tjhei
Copy link
Member

tjhei commented Jun 11, 2024 •
edited
Loading

Can you check:

  • cookbooks/inner_core_convection/?
  • cookbooks/subduction_initiation/

@gassmoeller gassmoeller force-pushed the make_gmg_default branch 2 times, most recently from 691cb12 to 571b090 Compare June 12, 2024 20:08
@tjhei
Copy link
Member

tjhei commented Jun 12, 2024

broken:

  • cookbooks/morency_doin_2004: linear solver failure
  • cookbooks/inner_core_convection: linear solver failure

@gassmoeller gassmoeller force-pushed the make_gmg_default branch 2 times, most recently from 88bef17 to 211c29a Compare June 13, 2024 00:50
@gassmoeller
Copy link
Member Author

I fixed the cookbooks by specifying AMG for now. Everything else is passing? Does that mean we are ready for flipping the big switch? 😄

@tjhei
Copy link
Member

tjhei commented Jun 13, 2024 •
edited
Loading

can you check:

  • cookbooks/subduction_initiation/
  • cookbooks/mantle_convection_with_continents_in_annulus/
  • cookbooks/crustal_deformation/
  • cookbooks/allken_et_al_2012_rift_interaction
  • benchmarks/buiter_et_al_2008_jgr (might work, takes a long time to finish in debug mode)

@gassmoeller
Copy link
Member Author

Ok, I have update the PR as discussed. I have adjusted two solver default settings to be more reasonable for GMG:

  • GMRES solver restart length = 100: The GMG solver needs less memory, so we can afford to increase the restart length. While it will make individual iterations more expensive, you can see in many tests that it decreases the iteration count by up to a factor of 2 if they need many iterations (e.g. 200+7 -> 55 in tests/adiabatic_plate_function). It should also benefit the iteration count for AMG
  • Number of cheap Stokes solver steps = 1000: The GMG cheap iterations do not have the same limitations as the AMG iterations, and GMG cheap should be the preferred iteration type. Increase the number to 1000 (= the same as our default expensive limit) to make sure we at least try the cheap iterations sufficiently, before switching to expensive.

Regarding the cookbooks:

  • subduction initiation and buiter_... and mantle_convection_with_continents_in_annulus work fine with the changes
  • allken_et_al_2012_rift_interaction and crustal_deformation do not converge with GMG. I changed both back to AMG.

I think I am happy with the current state. I would have to squash before merge, but I wanted to keep the test updates separate from each other for review. Opinions?

@tjhei
Copy link
Member

tjhei commented Jun 13, 2024

I think this is good to go now, thank you Rene!

@gassmoeller gassmoeller mentioned this pull request Jun 13, 2024
Open
2 tasks
@tjhei tjhei merged commit c91dbb6 into geodynamics:main Jun 13, 2024
8 checks passed
@gassmoeller
Copy link
Member Author

🎉 What a relief.

@gassmoeller gassmoeller deleted the make_gmg_default branch June 13, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjhei tjhei tjhei approved these changes

Assignees
No one assigned
Labels
ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gassmoeller @bangerth @tjhei