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

Revive SSPKnoth #292

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Revive SSPKnoth #292

merged 1 commit into from
Jun 12, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented May 24, 2024

This PR brings back SSPKnoth. In its current incarnation, SSPKnoth is a 2nd-order Rosenbrock method that does not require a Newton solver. The implementation was tested with a series of convergence tests and in ClimaAtmos. The convergence tests show the expected order of convergence when SSPKnoth is used fully implicitely with the correct Jacobian. This is not the case when the the Jacobian is approximated.

In ClimaAtmos, I investigated maximium timestep and best dissipation parameters for baroclinic waves. I found that the CAM_SE parameters work well and yield a maximum timestep of 200s for our target resolution (30 horizontal elements, 63 verfical ones).

In this PR, I also fixed a couple of small things that outdated.

I also added a general tutorial on how to use ClimaTimeSteppers, but the tutorial should be expanded with more information and explanations by those who know how things work.

Closes #276

@Sbozzolo Sbozzolo force-pushed the gb/rosenbrock branch 17 times, most recently from 250e00a to 6280071 Compare May 31, 2024 18:06
@Sbozzolo Sbozzolo marked this pull request as ready for review May 31, 2024 18:08
@Sbozzolo Sbozzolo force-pushed the gb/rosenbrock branch 4 times, most recently from 0ce232a to 30b50e6 Compare June 1, 2024 17:27
@Sbozzolo Sbozzolo force-pushed the gb/rosenbrock branch 5 times, most recently from ec07ffa to 6b747e7 Compare June 4, 2024 20:43
@Sbozzolo
Copy link
Member Author

Sbozzolo commented Jun 5, 2024

@dennisYatunin this is ready for review when you have time

@Sbozzolo Sbozzolo force-pushed the gb/rosenbrock branch 2 times, most recently from afdf9f4 to 04aa7f5 Compare June 6, 2024 16:00
Copy link
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

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

A couple of comments/questions, but this looks really great overall. Thanks for adding the tutorial as well!

docs/src/algorithm_formulations/rosenbrock.md Outdated Show resolved Hide resolved
docs/src/tutorials/diffusion.jl Outdated Show resolved Hide resolved
docs/src/tutorials/diffusion.jl Outdated Show resolved Hide resolved
docs/src/tutorials/diffusion.jl Outdated Show resolved Hide resolved
docs/src/tutorials/diffusion.jl Outdated Show resolved Hide resolved
docs/src/tutorials/diffusion.jl Outdated Show resolved Hide resolved
ext/benchmark_utils.jl Outdated Show resolved Hide resolved

Refer to the documentation for the precise meaning of the symbols below.
"""
struct RosenbrockTableau{N}
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to keep the tableau structs confined to the minimal set of values needed to define a timestepping scheme, and to have all values generated from those confined to the cache. The ExplicitTableau (which I'll rename to ButcherTableau in the reformulation) has the vector b, which you defined elsewhere in this file, and the matrix A, which corresponds to your matrices α and Γ. The ExplicitTableau also has the vector c, which I think corresponds to the values αi and γi that you compute in your stage loop; it might be worth pre-computing those here as well. So, maybe the constructor here can look something like RosenbrockTableau(α, Γ, b, c = sum(α, dims = 2), γ = sum(Γ, dims = 2))?

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 added constructor that takes alpha, Gamma, and b. Maybe we can refine things once we work with the reformulation?

src/solvers/rosenbrock.jl Outdated Show resolved Hide resolved
src/solvers/rosenbrock.jl Show resolved Hide resolved
@Sbozzolo
Copy link
Member Author

Documentation fails, but also fails on main with the same error. This is probably #283. I'll go ahead and merge this.

I'll also refine the tableau when things are a little bit more clear for the reformulation.

@Sbozzolo Sbozzolo merged commit c9ad882 into main Jun 12, 2024
7 of 8 checks passed
@Sbozzolo Sbozzolo deleted the gb/rosenbrock branch June 12, 2024 17:31
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.

Modernize SSPKnoth and ensure we can run it
2 participants