-
Notifications
You must be signed in to change notification settings - Fork 3
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
Online Diagnostics Module #206
Conversation
f7b8c90
to
837199a
Compare
6d60409
to
2bbfe9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! just minor comments :)
@@ -71,7 +72,7 @@ if isinteractive() | |||
parsed_args["vert_diff"] = true #hide | |||
parsed_args["rad"] = "gray" #hide | |||
parsed_args["microphy"] = "0M" #hide | |||
parsed_args["energy_check"] = true | |||
parsed_args["energy_check"] = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need #hide
here?
# Arguments | ||
- `cs`: [CoupledSimulation] containing info about the simulation | ||
""" | ||
trigger_callback(cs::CoupledSimulation, ::Monthly) = cs.dates.date[1] >= cs.dates.date1[1] ? true : false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this need the ::Monthly
param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it's more like a label for clarity. But in the future, we may want to have callbacks for Daily
or EveryTimestep
. This callback interface will probably change though once we start thinking about more complex timestepping.
# Atmos diagnostics | ||
|
||
""" | ||
get_var(cs, ::Val{:T}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add type to function definition in comment (for all get_var
functions)
src/Diagnostics.jl
Outdated
@@ -0,0 +1,215 @@ | |||
#= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change #=
, =#
to """
per Valeria's recent docstring updates
src/Diagnostics.jl
Outdated
|
||
|
||
""" | ||
pre_save(::TimeMean, cs::CoupledSimulation, dg::DiagnosticsGroup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing closing )
(and in line 196)
save_time_format | ||
|
||
|
||
FT = Float64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could add loop for FT in (Float32, Float64)
over all testsets to test both FTs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about it too, but then saw that it's not really testing any fundamental dependencies on the FT, so I figured Float64 suffices. Trying to cut down on unnecessary CI ;)
2bbfe9a
to
c080818
Compare
tests cleanup test fix check in test Manifest check in test Manifest temp temp dep temp clean temp fix dep fix dep fix dep fix test try format rebase fixes mpi test rebase clean amip data name fix rev fix dep fix dep fix dep fix
358054e
to
5dda1fc
Compare
bors r+ |
Purpose
Modularize the diagnostics utility.
To-do
trigger_callback
(for better tracking of callback types) andMonthly()
Note
Review checklist