Skip to content
This repository has been archived by the owner on Mar 1, 2023. It is now read-only.

Add SplitExplicitStepper with decoupled spin-down test #1257

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Add SplitExplicitStepper with decoupled spin-down test #1257

merged 1 commit into from
Jun 23, 2020

Conversation

blallen
Copy link
Contributor

@blallen blallen commented Jun 18, 2020

Description

This PR adds the MultistateMultirateRungeKutta solver from the branches bla/multistate_multirate and jmc/split_explicit_dvlp in a new form as the SplitExplicitStepper. A simple test of running the HBModel and SWModel in parallel performing the hydrostatic spindown test. Both models converge to the analytic solution. A separate PR will be added that couples the two models with this test.

Originally, I attempted to include a working version of the add_fast_steps option from jmc/split_explicit_dvlp; however, that caused the SWModel to not converge properly. This feature will be revisited in a future PR.

I have

  • Written and run all necessary tests with CLIMA by including tests/runtests.jl
  • Followed all necessary style guidelines and run julia .dev/climaformat.jl .
  • Updated the documentation to reflect changes from this PR.

For review by CLIMA developers

  • There are no open pull requests for this already
  • CLIMA developers with relevant expertise have been assigned to review this submission
  • The code conforms to the style guidelines and has consistent naming conventions. julia .dev/format.jl has been run in a separate commit.
  • This code does what it is technically intended to do (all numerics make sense physically and/or computationally)

@blallen blallen added Ocean 🌊 "How inappropriate to call this planet Earth when it is quite clearly Ocean" Timestepping tests labels Jun 18, 2020
@blallen blallen self-assigned this Jun 18, 2020
@blallen
Copy link
Contributor Author

blallen commented Jun 18, 2020

bors try

bors bot added a commit that referenced this pull request Jun 18, 2020
@bors
Try #1257:
265c04a
@bors
Copy link
Contributor

bors bot commented Jun 18, 2020

try

Build failed:

@blallen
Copy link
Contributor Author

blallen commented Jun 18, 2020

bors try

bors bot added a commit that referenced this pull request Jun 18, 2020
@bors
Try #1257:
0bad175
@bors
Copy link
Contributor

bors bot commented Jun 18, 2020

try

Build failed:

jkozdon
jkozdon reviewed Jun 18, 2020
View reviewed changes
src/Numerics/DGMethods/DGModel.jl Outdated
nrealelem = length(topology.realelems)
nhorzrealelem = div(nrealelem, nvertelem)

return (Nq, Nqk, Nfp, Np, nvertelem, nhorzelem, nhorzrealelem, nrealelem)
Copy link

Choose a reason for hiding this comment

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

Would a named tuple be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably

jkozdon
jkozdon reviewed Jun 18, 2020
View reviewed changes
test/Ocean/runtests.jl Outdated Show resolved Hide resolved
jkozdon
jkozdon reviewed Jun 18, 2020
View reviewed changes
src/Numerics/ODESolvers/SplitExplicitMethod.jl Outdated

LSRK2N = LowStorageRungeKutta2N

"""
Copy link

Choose a reason for hiding this comment

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

Since this is a custom / home baked stepper might be worth documenting mathematically what it's doing. (Just thinking that our future selves will need to understand it, and we will forget.)

Copy link

Choose a reason for hiding this comment

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

Am I correct that this is a very simple first order splitting of the ODE?

dQ = f(Q,P)
dP = g(Q, P)

and you are doing something like:

dQ = f(Q, P^n)

to take Q^n to `Q^{n+1} and then

dP = g(Q^{n+1}, P)

to take P^n to P^{N+1}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this sounds about right, @sandreza and @jm-c can you comment further?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the latest commit 287c821 shows the details of the splitting by filling in the communication functions. there's still some work needed before this can be used, because we need to add two auxiliary balance laws in order to do the vertical integrals needed, but the layout of what is happening is there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will address in #1268

jkozdon
jkozdon reviewed Jun 18, 2020
View reviewed changes
src/Numerics/ODESolvers/SplitExplicitMethod.jl
end

# Compute (and RK update) slow tendency
slow.rhs!(dQslow, Qslow, param, slow_stage_time, increment = true)
Copy link

Choose a reason for hiding this comment

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

I think you can replace this with something like

Suggested change
slow.rhs!(dQslow, Qslow, param, slow_stage_time, increment = true)
dQslow .+= dQ2fast

Copy link

Choose a reason for hiding this comment

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

(unless Qslow is modified in tendency_from_slow_to_fast)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jm-c can you test this change in your branch?

jkozdon
jkozdon reviewed Jun 18, 2020
View reviewed changes
Copy link

@jkozdon jkozdon left a comment

Choose a reason for hiding this comment

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

I don't see any glaring problems.

Not sure there are enough comments to help us remember what the goal of everything is.

(Trying to think of our future selves here)

@blallen
Copy link
Contributor Author

blallen commented Jun 18, 2020

I don't see any glaring problems.

Not sure there are enough comments to help us remember what the goal of everything is.

(Trying to think of our future selves here)

I think most of your questions/concerns about comments are about the parts of the timestepper that are stubs in this PR and will be filled out in the next one.

I can also just wait and include those in this PR. I mostly wanted this decoupled test in first, but I guess it will be in all the CI even if I add the more complicated stuff here.

@blallen
Copy link
Contributor Author

blallen commented Jun 18, 2020

bors try

@blallen blallen force-pushed the bla/split-explicit-stepper branch from b4a6daf to 8e8c1c8 Compare June 22, 2020 17:36
@blallen blallen mentioned this pull request Jun 22, 2020
Merged
7 tasks
@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

#1266 scares me and I want the basics of this in sooner rather than later. Communication has been moved to #1268 and a necessary change in the SWModel has been moved to #1267.

Going to try to merge this soon.

@blallen blallen marked this pull request as ready for review June 22, 2020 17:52
@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 22, 2020

Merge conflict.

@blallen blallen force-pushed the bla/split-explicit-stepper branch from 8e8c1c8 to 3de0ae3 Compare June 22, 2020 20:14
@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

bors r+

@blallen blallen force-pushed the bla/split-explicit-stepper branch from 3de0ae3 to f4ec39c Compare June 22, 2020 20:49
@bors
Copy link
Contributor

bors bot commented Jun 22, 2020

Canceled.

@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

bors r+

@blallen blallen force-pushed the bla/split-explicit-stepper branch from f4ec39c to 5da8511 Compare June 22, 2020 21:13
@bors
Copy link
Contributor

bors bot commented Jun 22, 2020

Canceled.

@blallen
Add SplitExplicitStepper with decoupled spin-down test
1b833e5 
port over MultirateMultistateRK solver as SplitExplicitSolver

decoupled models run, slow model matches analytical solution

Revert additional steps code, fast model converges properly, add tests

port bugfixes from CliMA#1256

Incorporate Jeremy's comments

fix docstring

Co-Authored-By: Jean-Michel Campin <jm-c@users.noreply.github.com>
Co-Authored-By: sandreza <andrenogueirasouza@gmail.com>
Co-Authored-By: Jeremy E Kozdon <jekozdon@nps.edu>

finish rebase
@blallen blallen force-pushed the bla/split-explicit-stepper branch from 5da8511 to 1b833e5 Compare June 22, 2020 21:51
@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

bors r+

bors bot added a commit that referenced this pull request Jun 22, 2020
@bors @blallen
Merge #1257
1824ba5 
1257: Add SplitExplicitStepper with decoupled spin-down test r=blallen a=blallen

# Description

This PR adds the MultistateMultirateRungeKutta solver from the branches `bla/multistate_multirate` and `jmc/split_explicit_dvlp` in a new form as the SplitExplicitStepper. A simple test of running the `HBModel` and `SWModel` in parallel performing the hydrostatic spindown test. Both models converge to the analytic solution. A separate PR will be added that couples the two models with this test.

Originally, I attempted to include a working version of the `add_fast_steps` option from `jmc/split_explicit_dvlp`; however, that caused the `SWModel` to not converge properly. This feature will be revisited in a future PR. 



Co-authored-by: Brandon Allen <ballen@mit.edu>
@charleskawczynski
Copy link
Member

bors r-

@bors
Copy link
Contributor

bors bot commented Jun 22, 2020

Canceled.

@charleskawczynski
Copy link
Member

@blallen sorry to stop this! CI on master is currently broken! 😬 We're going to try to fix it before merging anything else!

@blallen
Copy link
Contributor Author

blallen commented Jun 22, 2020

@charleskawczynski damn, I was trying to avoid shenanigans from #1266 and instead got shenanigans from #1050. Well at least this PR is finished and the active development is elsewhere now :)

@charleskawczynski
Copy link
Member

@charleskawczynski damn, I was trying to avoid shenanigans from #1266 and instead got shenanigans from #1050. Well at least this PR is finished and the active development is elsewhere now :)

Sorry, we should be able to merge right after #1273.

@jkozdon
Copy link

jkozdon commented Jun 23, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 23, 2020

Build succeeded:

@bors bors bot merged commit ab3cd0d into CliMA:master Jun 23, 2020
@blallen blallen mentioned this pull request Jun 23, 2020
Open
3 tasks
bors bot added a commit that referenced this pull request Jul 21, 2020
@bors @blallen
Merge #1268
4d6ae25 
1268: Couple the HBModel and SWModel for use in the Split-Explicit Stepper r=blallen a=blallen

# Description

A continuation of #1257, adding the necessary communication between the two models needed to have a true split-explicit stepper. The work in this PR enables us to run with up to a 90-minute timestep for the hydrostatic spindown test case. 

The broadcasts of reshaped MPIStateArrays depends on #1071 to run on the GPU. I'll pull over the commits from that PR for running bors, but we need to merge that before we merge this one.

There's quite a bit of partial work in this PR that was necessary to get things running, but that either isn't fully implemented everywhere it should be or isn't implemented super elegantly (mainly regarding collecting all the Ocean models into a single module and how multiple dispatch is used for the couple). I'll revisit that once we have the full timestepping machinery running.



Co-authored-by: Brandon Allen <ballen@mit.edu>
bors bot added a commit that referenced this pull request Jul 23, 2020
@bors @blallen
Merge #1268
0c70054 
1268: Couple the HBModel and SWModel for use in the Split-Explicit Stepper r=blallen a=blallen

# Description

A continuation of #1257, adding the necessary communication between the two models needed to have a true split-explicit stepper. The work in this PR enables us to run with up to a 90-minute timestep for the hydrostatic spindown test case. 

The broadcasts of reshaped MPIStateArrays depends on #1071 to run on the GPU. I'll pull over the commits from that PR for running bors, but we need to merge that before we merge this one.

There's quite a bit of partial work in this PR that was necessary to get things running, but that either isn't fully implemented everywhere it should be or isn't implemented super elegantly (mainly regarding collecting all the Ocean models into a single module and how multiple dispatch is used for the couple). I'll revisit that once we have the full timestepping machinery running.



Co-authored-by: Brandon Allen <ballen@mit.edu>
@blallen blallen deleted the bla/split-explicit-stepper branch July 23, 2020 19:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkozdon jkozdon jkozdon left review comments

@thomasgibson thomasgibson thomasgibson left review comments

@christophernhill christophernhill Awaiting requested review from christophernhill

@jm-c jm-c Awaiting requested review from jm-c

Assignees

@blallen blallen

Labels
Ocean 🌊 "How inappropriate to call this planet Earth when it is quite clearly Ocean" tests Timestepping
Projects
None yet
Milestone
ClimateMachine.jl v0.2.0 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@blallen @charleskawczynski @jkozdon @thomasgibson