-
Notifications
You must be signed in to change notification settings - Fork 195
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
AbstractSchedules for scheduling output and diagnostics #1070
Conversation
I think |
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.
This is awesome! Much cleaner interface with tons of uses.
For now I guess you can only have one schedule per output writer. I think this is fine: if users want to specify multiple schedules they can create multiple output writers (or just define a custom schedule(model)
. I.e. we shouldn't worry about this.
Thanks for updating the documentation on output writing, it was quite out of date and it's not the most enjoyable work. Also nice work on refactoring the NetCDFOutputWriter
through multiple disptach!
Super pedantic but Oceananigans.Util
now defines minutes
and hours
so we can use e.g. TimeInterval(2minutes)
instead of TimeInterval(2minute)
but maybe this is bad as it could signal a proliferation of exported names .
```@docs | ||
NetCDFOutputWriter | ||
``` |
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.
💯
Co-authored-by: Ali Ramadhan <[email protected]>
Right, that's default! I think it makes sense. |
Is this for examples and docs? It does obviously read better; I'm not sure about the downside of name proliferation. Maybe we can stew on it and update in a future PR? |
…anigans.jl into glw/flexible-output-criteria
Just updated a few remaining jldoctests so tests should pass soon. |
I was thinking both, i.e. encourage their use to improve script readability. Yeah we can just update things as we go along. I guess the downside is that new users may not immediately notice that these common words, e.g. |
very much agree. |
Yeah, I think eventually or if we get lots of requests it might make sense to add some kind of |
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
- Coverage 57.70% 57.28% -0.43%
==========================================
Files 158 164 +6
Lines 3807 4139 +332
==========================================
+ Hits 2197 2371 +174
- Misses 1610 1768 +158
Continue to review full report at Codecov.
|
This PR provides a substantial rewrite to the user API for "scheduling" output and diagnostics.
Previously, output was scheduled by specifying one of two keyword arguments when constructing the
AbstractOutputWriter
:time_interval
, anditeration_interval
. An example from the documentation isTime-averaging was specified by providing one or two additional keyword arguments:
time_average_interval
and (optionally)time_average_stride
.This PR eliminates these keyword arguments in favor of a single argument
schedule
. In general, theschedule
is an object which returnstrue
(when output should be written, or a diagnostic calculated) andfalse
(otherwise).This PR implements four
AbstractSchedule
s, and more are possible:TimeInterval
: periodic schedule that reoccurs on a interval of model time.IterationInterval
: periodic schedule that reoccurs on an iteration of model iterationsWallClockInterval
periodic schedule that reoccurs on an interval of "wall time", as kept by the clock on your wallAveragedTimeInterval
: periodic schedule that reoccurs on an interval of model time, and specifies time-averaging of output over awindow
(and a defaultwindow
equal tointerval
is now provided for friendliness)The old syntax thus becomes
and averaging is specified (with a time window of 20 seconds) by writing
This PR starts updating the examples and docs, though we still need to
Since we now have a fairly convincing test about the accuracy of
WindowedTimeAverage
, I think we should alsoWindowedTimeAverage
which is apropos to this PR since it changes the syntax through which averaging is specified.
Eventually, we should use the "scheduling" concept for both callafter function in
run!(simulation)
and theTimeStepWizard
(separately), which will get rid of theiteration_interval
argument inrun!(simulation)
.PS: should we use
WindowAveragedTimeInterval
rather thanAveragedTimeInterval
? I wasn't sure if the added verbosity added clarity here (the important part is the "Averaging") but I'm open to considering it.Resolves #1019
Resolves #853
Resolves #845