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

Butcher Tableau #413

Merged
merged 19 commits into from
Aug 31, 2023
Merged

Butcher Tableau #413

merged 19 commits into from
Aug 31, 2023

Conversation

atb1995
Copy link
Collaborator

@atb1995 atb1995 commented Jul 31, 2023

We have created a new time stepper class ExplicitMultistage which takes in a Butcher Tableau in a numpy array of the form: [a_00 0 . 0 ]
[a_10 a_11 . 0 ]
[ . . . . ]
[ b_0 b_1 . b_{s-1}]
to run single time step multistage explicit time discretisation's. This allows time steppers such as ForwardEuler, RK4, SSPRK3 and Heun to be rewritten by the use of a single Butcher Tableau. It will also allow explicit multistage schemes to be easily written in future.

Unfortunately two bugs were found in the process. The first is that the Fallout physics scheme has a few bugs in, meaning test_precipitation.py fails. I have xfailed this pytest for now. This problem is detailed in issue #334.

The second was that RK4 on main fails in some pytests when using the NonlinearVariationalSolver. This failure translated to all ExplicitMultistage as they are written in the same way as RK4. I have changed to LinearVariationalSolver in ExplicitMultistage, and have created a minimal failing example outside of Gusto, which I will raise with Firedrake team.

@atb1995 atb1995 linked an issue Jul 31, 2023 that may be closed by this pull request
Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

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

This looks really neatly done to me. I think the main thing is to make sure that we're including the physics source evaluations (which were added in after you started this branch)

gusto/time_discretisation.py Outdated Show resolved Hide resolved
@@ -315,123 +331,55 @@ def apply(self, x_out, x_in):
x_out.assign(self.x1)


class ForwardEuler(ExplicitTimeDiscretisation):
class ExplicitMultistage(ExplicitTimeDiscretisation):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind ExplicitMultistage, but have also wondered if ExplicitRungeKutta is clearer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I may veto this.. I'm keener on multistage (especially in contrast to multistep)

self.k1.assign(self.x_out)
self.x1.assign(x_in + 0.5 * self.dt * self.k1)
def solve_stage(self, x0, stage):
self.x1.assign(x0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to include the call to evaluate physics terms here (this was included after you branched off from trunk!). But you should check if makes sense to evaluate the physics sources at exactly this point...

Suggested change
self.x1.assign(x0)
self.x1.assign(x0)
for evaluate in self.evaluate_source:
evaluate(self.x1, self.dt)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added this!

self.x1.assign(x_in + 0.5 * self.dt * self.k2)
if (stage == self.nStages - 1):
self.x1.assign(x0)
for i in range(self.nStages):
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we need to add the physics source evaluation here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not required here, as this is the final timestep evaluation of field (after solve)

super().__init__(domain, field_name=field_name, subcycles=subcycles,
solver_parameters=solver_parameters,
limiter=limiter, options=options, butcher_matrix=butcher_matrix)
self.butcher_matrix = np.array([[1., 0., 0.], [1./4., 1./4., 0.], [1./6., 1./6., 2./3.]])
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like how clear this is now

tommbendall
tommbendall previously approved these changes Aug 29, 2023
Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

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

I'm happy with this, but suggest including "Runge-Kutta" in the docstring for the new class

gusto/time_discretisation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jshipton jshipton left a comment

Choose a reason for hiding this comment

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

The only thing is to take care with line length, otherwise this looks great - thanks @atb1995 !

Copy link
Contributor

@jshipton jshipton left a comment

Choose a reason for hiding this comment

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

Looks great - thanks @atb1995 !

@jshipton jshipton merged commit 2ced7c8 into main Aug 31, 2023
4 checks passed
@jshipton jshipton deleted the butcher_tableau branch August 31, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiter should be applied after each stage of RK method
4 participants