Add new integration #171
Add new integration #171
Conversation
Changed the id and path of the output to match the way SpeedyWeather does it
Adding new runid
Matching the upstream repo
Just correcting the error with path versus outpath, restoring the name initpath in Parameters, all else the same
Matches the results from before changing the code
fixed that here
|
I'm realizing this actually already includes the changes I made to output.jl, I didn't realize that originally...I just checked and it does seem to work, but I remember that last time I messed with this I broke something on your end? |
| parameter.txt | ||
|
|
||
| # ignoring a folder on my computer (SW) | ||
| /my_scripts |
There was a problem hiding this comment.
This should probably be reverted for general users
There was a problem hiding this comment.
I agree, but I admittedly don't know how to push my code without having this line in the .gitignore (if I don't want that folder online). Do you usually manually remove it on Github?
There was a problem hiding this comment.
You can
- revert changes, push, change locally again, don't push again
- press
.here, that opens a VS code session for this branch in which you can change things and push them without local changes
| Dates = "ade2ca70-3891-5945-98fb-dc099432e06a" | ||
| Interpolations = "a98d9a8b-a2ab-59e6-89dd-64a1c18fca59" | ||
| NetCDF = "30363a11-5582-574a-97bb-aa9a979735b9" | ||
| Parameters = "d96e819e-fc66-5662-9728-84c9c7592b0a" | ||
| Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80" |
There was a problem hiding this comment.
Both Checkpointing and Plots (and Enzyme) should probably be an extension? That way they are only a soft dependency and a general user does not depend on them but you can use/extend that functionality without a hard dependency on it, Oceananigans does that for Enzyme, SpeedyWeather does that for Makie
There was a problem hiding this comment.
I can remove Plots, but if I don't use Checkpointing I won't be able to run include(".src/ShallowWaters.jl"). I'll elaborate in a different comment
| # struct DiagnosticVars{T,Tprog} | ||
| # RungeKutta::RungeKuttaVars{Tprog} | ||
| # Tendencies::TendencyVars{Tprog} | ||
| # VolumeFluxes::VolumeFluxVars{T} | ||
| # Vorticity::VorticityVars{T} | ||
| # Bernoulli::BernoulliVars{T} | ||
| # Bottomdrag::BottomdragVars{T} | ||
| # ArakawaHsu::ArakawaHsuVars{T} | ||
| # Laplace::LaplaceVars{T} | ||
| # Smagorinsky::SmagorinskyVars{T} | ||
| # SemiLagrange::SemiLagrangeVars{T} | ||
| # PrognosticVarsRHS::PrognosticVars{T} # low precision version | ||
| # end |
There was a problem hiding this comment.
I see you moved them into src/model_setup.jl that's fine but then I'd actually delete them here not just uncomment. I only leave commented code in when to document that some functionality could be added here at some point in the future or to explain with code what's going on
There was a problem hiding this comment.
Also forgot to re-delete this
|
|
||
| # In order to run the following you need to change lines in model_setup.jl | ||
|
|
||
| function mk_run_model(::Type{T}=Float32; # number format |
There was a problem hiding this comment.
Sorry I don't understand how mk_run_model is different from run_model. I'm happy for you to change the run_model function directly, you can provide keyword arguments to return the same values as before (or not)
There was a problem hiding this comment.
mk_run_model left all the structures as is (didn't add Prog and Diag to S). I only added this to check that the integration in both cases returns the same thing (both with single struct S and separate structs), I can re-remove it!
| @unpack nstep_advcor,nstep_diff,nadvstep,nadvstep_half = S.grid | ||
|
|
||
| # calculate layer thicknesses for initial conditions | ||
| ShallowWaters.thickness!(Diag.VolumeFluxes.h,η,S.forcing.H) |
There was a problem hiding this comment.
Why is the ShallowWaters. necessary here? Are we not in the same module still?
There was a problem hiding this comment.
It's not, this was a cut-and-paste from different code and I forgot to delete it
|
|
||
| end | ||
|
|
||
| function loop(S,scheme) |
There was a problem hiding this comment.
It feels like time_integration() and loop() have a lot of code redundancy. Is there no way to have only one possibly separating out functionality from one with some if statements?
There was a problem hiding this comment.
Ah, so there are two different integration functions now. One is time_integration, and this is what gets run when someone does ShallowWaters.run_model, and the other is checkpointed_integration, which separately calls loop inside. loop runs the time integration loop and checkpointed_integration calls it. checkpointed_integration should only be used separately by a user wishing to differentiate the model. I don't remember if it's strictly necessary to split the two, but somewhere in debugging Checkpointing they got split and I never put them back together
There was a problem hiding this comment.
Could you try to put them together? From a software enginereeing perspective it's not wise to have two large functions that are conceptually doing the same
There was a problem hiding this comment.
Mmm maybe, let me see something
|
|
||
| function loop(S,scheme) | ||
|
|
||
| @checkpoint_struct scheme S for S.parameters.i = 1:S.grid.nt |
There was a problem hiding this comment.
This is the problem line. If I don't add Checkpointing then running include("./src/ShallowWaters.jl") will fail because @checkpoint_struct will be undefined. In response to a more recent comment, I might still be able to integrate the function as normal with this, but I haven't tried this myself and it might also still run checkpointing? I'm not sure what exactly will change
There was a problem hiding this comment.
I'm happy to find an alternative place to put this function though! Maybe create it's own script and instruct users to include that script separately if they want to checkpoint?
There was a problem hiding this comment.
Then in time_integration.jl I'd leave the single definition for model integration
Added new functions in /src that change the integration into taking only a single struct
S. I've checked that the integration returns the same results as the original code (before taking a single struct), and it seems to. I did change some default parameters around but I don't know if this matters or not. So people who download ShallowWaters.jl should still be able to run something likeShallowWaters.run_model(stuff)and it'll work as before (at least it does for me), but please also check this on your end!I also added a couple new packages to ShallowWaters.jl itself, namely Checkpointing and Plots (Plots was mostly for me to look at results I was getting). I haven't yet added Enzyme, that'll be the next pull request I do. I'm also going to re-attempt to fix the output issues into what works for me.
Let me know if I need to adjust stuff in my branch!