Create a GMRES subsection in Solver parameters #2163

naliboff · 2018-04-12T22:32:24Z

Partially addresses #2023. Moving these two parameters does not require running the update script, but moving any of the other solver parameters would require doing so.

jdannberg · 2018-04-12T22:38:11Z

Keep in mind that even though we may not use any of these parameters in our tests, other people might still use them in their input file, so we should add an entry in the update script anyway. Also, I would vote for moving Number of cheap Stokes solver steps and Maximum number of expensive Stokes solver steps too. Thoughts?

naliboff · 2018-04-12T22:46:11Z

I agree we should move Number of cheap Stokes solver steps and Maximum number of expensive Stokes solver steps. Beyond these two, I think we would need to move all of the rest as a group. For example, I don't think it would make sense to have the Nonlinear solver scheme and Nonlinear solver tolerance in different sections.

For these (and any other) parameters I don't think there is any need to use an additional subsection under Solver parameters.

Once we decide on the remaining parameters to move, I'll modify the update script.

bangerth · 2018-04-12T23:25:02Z

source/simulator/parameters.cc

@@ -338,6 +321,26 @@ namespace aspect

    prm.enter_subsection ("Solver parameters");
    {
+      prm.enter_subsection ("GMRES parameters");


Could I advocate for this to be renamed "Linear solver parameters"? The fact that we use GMRES is not really material in this regard -- it's just what we happen to use -- whereas the important fact is that it's about solving the linear problem.

Sounds good. I think it would be logical to move Number of cheap Stokes solver steps and Maximum number of expensive Stokes solver steps into this section as well.

That makes sense.

jdannberg · 2018-04-13T00:23:29Z

Thinking about this for a bit, we could also discuss if we want to move Use direct solver for Stokes system too.

naliboff · 2018-04-13T20:37:28Z

If we keep 'Linear solver tolerance' outside of the Solver parameters subsection, I think it makes sense to keep Use direct solver for Stokes system in the same place. However, If the group consensus is to move it, no real objection.

naliboff · 2018-04-13T21:53:21Z

I've updated the subsection name to Linear solver parameters and moved Number of cheap Stokes solver steps and Maximum number of expensive Stokes solver steps here.

Unless there are more parameters to move into this subsection or Solver parameters in general, I'll revise the update script. Many of the tests should fail before this update.

bangerth · 2018-04-14T00:40:56Z

On 04/13/2018 02:37 PM, naliboff wrote: If we keep 'Linear solver tolerance' outside of the Solver parameters subsection, I think it makes sense to keep |Use direct solver for Stokes system| in the same place. However, If the group consensus is to move it, no real objection.

I'd say move both. They are both clearly related to the linear solver.

gassmoeller

Two minor improvements. I will send you an update script for the parameter files later this week.

gassmoeller · 2018-04-17T23:20:18Z

source/simulator/parameters.cc

@@ -338,6 +255,92 @@ namespace aspect

    prm.enter_subsection ("Solver parameters");
    {
+      prm.enter_subsection ("Linear solver parameters");


Could you rename this "Stokes solver parameters"?

gassmoeller · 2018-04-17T23:20:44Z

source/simulator/parameters.cc

+                           Patterns::Double(0,1),
+                           "The relative tolerance up to which the linear system for "
+                           "the composition system gets solved. See `linear solver "
+                           "tolerance' for more details.");


And leave these for now in the global section.

naliboff · 2018-04-18T17:42:24Z

@gassmoeller - The last commit addressed your two comments. Should I squash everything down to one commit before changing the update script?

gassmoeller

Nice job. Now we just need to figure out the test failures.

gassmoeller · 2018-04-19T23:20:17Z

cookbooks/global_no_melt.prm

-# We fix the temperature at the top and bottom boundaries of
-# the box, and prescribe free-slip boundary conditions on all
-# sides. 
+# We choose a high tolerance for the linear Stokes solver.


strict tolerance might be less ambiguous

gassmoeller · 2018-04-23T17:56:59Z

I think I fixed the update script for all special cases we had. @naliboff, sorry for taking over your PR, I rebased everything after #2168 was merged, so you will need to throw away your local branch and check out this one if you want to do any more changes. Hopefully that should not be necessary though. I removed some parameter lines that just set parameters to their default value, and made sure that the benchmarks and cookbooks are still nice to read.
Time for a final review and merge?

jdannberg · 2018-04-23T18:21:14Z

cookbooks/global_no_melt.prm


 # The end time of the simulation. Because we want to see how upwellings
 # and downwellings evolve over time, and if differences develop between 
 # the model with and without melt migration, we set the end time to 140 Ma.
 set End time                               = 1.4e8

+# We choose a stricter than default linear Stokes solver tolerance,
+# to improve the nonlinear convergence behavior.


This model does not do any nonlinear iterations. And please don't ask, I do not remember why I set the parameter to this number... (except to make sure that the global_melt and global_no_melt models have the same parameter values). So I would either remove the comment, or move it to the global_melt.prm input file.

This would also have the advantage that the files in the doc/manual/cookbooks and the cookbooks folder have the same documentation.

gassmoeller · 2018-04-23T18:38:12Z

Done. @tjhei: It seems the tester is taking a break, can you have a look? Thanks!

naliboff · 2018-04-23T18:42:08Z

@gassmoeller - No problem at all and thanks for doing all the work with the update script and parameter file cleanup! I just looked through the cookbook and benchmark parameter file changes and all looks good. As soon as the tester finishes, I agree this is ready to merge.

jdannberg · 2018-04-23T18:42:47Z

I am doing a review at the moment, please don't merge until I'm done.

jdannberg

Thanks for fixing this! Please address my comments before merging.

jdannberg · 2018-04-23T18:30:56Z

cookbooks/global_no_melt.prm


 # The end time of the simulation. Because we want to see how upwellings
 # and downwellings evolve over time, and if differences develop between 
 # the model with and without melt migration, we set the end time to 140 Ma.
 set End time                               = 1.4e8

+# We choose a stricter than default linear Stokes solver tolerance,
+# to improve the nonlinear convergence behavior.


This would also have the advantage that the files in the doc/manual/cookbooks and the cookbooks folder have the same documentation.

jdannberg · 2018-04-23T18:34:28Z

doc/manual/cookbooks/benchmarks/burstedde/burstedde.prm

+  subsection Stokes solver parameters
+    set Number of cheap Stokes solver steps = 0
+  end
+end


If this was not in the original file, you don't have to add this here (in particular as there's no documentation on why the parameter is chosen like this).

jdannberg · 2018-04-23T18:35:41Z

doc/manual/cookbooks/benchmarks/burstedde/burstedde.prm

@@ -1,4 +1,3 @@
-set Linear solver tolerance                = 1e-12


This is not the default value, so you can not just remove it. Please add it below to the Stokes solver parameters.

jdannberg · 2018-04-23T18:36:26Z

benchmarks/burstedde/burstedde.prm

@@ -9,7 +9,6 @@
 # equal to the start time to force a single time
 # step before the program terminates.
 set Additional shared libraries            = ./libburstedde.so
-set Linear solver tolerance                = 1e-12


This is not the default value, so you can not just remove it.

Good catch!

jdannberg · 2018-04-23T18:52:20Z

source/simulator/parameters.cc

-                       "If this value is set too low for the size of the problem, the Stokes solver will "
-                       "not converge and return an error message pointing out that the user didn't allow "
-                       "a sufficiently large number of iterations for the iterative solver to converge.");
-
    prm.declare_entry ("Temperature solver tolerance", "1e-12",


I thought we had agreed to also move the temperature and composition solver tolerance if we move the linear solver tolerance?
I am not insisting on this (and the last time we talked about this, I had assumed none of these tolerances would be moved, that is why I argued they should stay where they are), but I just think this can be quite confusing. Why is the Linear solver tolerance in its own subsection, while the temperature and composition solver tolerances aren't?
We can also do that in a follow-up pull request, but we should at least discuss again what the final state for the release should be.

I recall us deciding that the composition and temperature solver tolerance would be moved eventually, but not in this pull request.

I think the decision was based on the fact that there currently are no other values we would move into the composition and temperature solver subsections and it would simply add lines to the input file. If someone creates more parameters related to the composition and linear solver tolerance, then it would make sense to move them.

However, I agree it is a bit confusing to have the temperature and composition solver tolerances as global parameters now. Do you think it is worth moving them into a new subsection before the 2.0 release? If so, I vote to do this in a separate PR.

gassmoeller · 2018-04-23T19:22:25Z

I fixed the issues, thanks for taking a look!
Concerning the advection solver tolerances: I think we discussed this, and settled on something like this: If we move the advection solver tolerances, we also have to do X (I do not quite remember, I think you brought up the point, and I think it was something about the stabilization). That is why we decided to leave them at their current position for now.

jdannberg · 2018-04-23T22:22:27Z

Okay, looks good to me! I've created an issue (#2171) for the advection solver tolerances, which can then be handled in a follow-up pull request.
Go ahead and merge once the tester is running again and all tests have passed.

jdannberg added this to the 2.0 milestone Apr 12, 2018

bangerth reviewed Apr 12, 2018

View reviewed changes

naliboff force-pushed the create_gmres_subsection branch from 9e679a0 to 80268d4 Compare April 13, 2018 21:49

naliboff force-pushed the create_gmres_subsection branch from 2eafe1a to 4cea89b Compare April 17, 2018 22:37

gassmoeller reviewed Apr 17, 2018

View reviewed changes

gassmoeller mentioned this pull request Apr 19, 2018

Run update script #2166

Merged

naliboff force-pushed the create_gmres_subsection branch from f59b222 to 9bb8a1e Compare April 19, 2018 20:27

gassmoeller reviewed Apr 19, 2018

View reviewed changes

naliboff force-pushed the create_gmres_subsection branch from fa6a368 to 62d4045 Compare April 19, 2018 23:25

gassmoeller mentioned this pull request Apr 20, 2018

Reorganize update scripts #2168

Merged

gassmoeller force-pushed the create_gmres_subsection branch from 939a84f to 58b87aa Compare April 23, 2018 07:37

naliboff and others added 6 commits April 23, 2018 10:47

Create Stokes solver parameters subsection

fcaaa03

Extend update script

b949e0d

Automatically update prm files

c911e52

Manual parameter file changes

6a2a9ec

Fix maximum expensive steps

f102e6c

Add changelog

5a73601

gassmoeller force-pushed the create_gmres_subsection branch from 7a29baa to 5a73601 Compare April 23, 2018 17:48

Fix some manually messed up newlines

aacecb1

jdannberg reviewed Apr 23, 2018

View reviewed changes

Fix cookbook comments

58daf73

gassmoeller force-pushed the create_gmres_subsection branch from 164585a to 58daf73 Compare April 23, 2018 19:17

jdannberg mentioned this pull request Apr 23, 2018

Move temperature and composition solver tolerance into the Solver settings subsection #2171

Closed

jdannberg added the ready to merge label Apr 23, 2018

jdannberg merged commit debde66 into geodynamics:master Apr 24, 2018

jdannberg mentioned this pull request Apr 24, 2018

solver parameters #2023

Closed

naliboff deleted the create_gmres_subsection branch June 8, 2018 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a GMRES subsection in Solver parameters #2163

Create a GMRES subsection in Solver parameters #2163

naliboff commented Apr 12, 2018

jdannberg commented Apr 12, 2018

naliboff commented Apr 12, 2018

bangerth Apr 12, 2018

jdannberg Apr 13, 2018

naliboff Apr 13, 2018

bangerth Apr 13, 2018

jdannberg commented Apr 13, 2018

naliboff commented Apr 13, 2018

naliboff commented Apr 13, 2018

bangerth commented Apr 14, 2018 via email

gassmoeller left a comment

gassmoeller Apr 17, 2018

gassmoeller Apr 17, 2018

naliboff commented Apr 18, 2018

gassmoeller left a comment

gassmoeller Apr 19, 2018

gassmoeller commented Apr 23, 2018

jdannberg Apr 23, 2018

jdannberg Apr 23, 2018

gassmoeller commented Apr 23, 2018

naliboff commented Apr 23, 2018

jdannberg commented Apr 23, 2018

jdannberg left a comment

jdannberg Apr 23, 2018

jdannberg Apr 23, 2018

jdannberg Apr 23, 2018

jdannberg Apr 23, 2018

naliboff Apr 23, 2018

jdannberg Apr 23, 2018

naliboff Apr 23, 2018

gassmoeller commented Apr 23, 2018

jdannberg commented Apr 23, 2018

Create a GMRES subsection in Solver parameters #2163

Create a GMRES subsection in Solver parameters #2163

Conversation

naliboff commented Apr 12, 2018

jdannberg commented Apr 12, 2018

naliboff commented Apr 12, 2018

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

jdannberg commented Apr 13, 2018

naliboff commented Apr 13, 2018

naliboff commented Apr 13, 2018

bangerth commented Apr 14, 2018 via email

gassmoeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naliboff commented Apr 18, 2018

gassmoeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gassmoeller commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gassmoeller commented Apr 23, 2018

naliboff commented Apr 23, 2018

jdannberg commented Apr 23, 2018

jdannberg 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

gassmoeller commented Apr 23, 2018

jdannberg commented Apr 23, 2018