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

Put Diagnostics and OutputWriters into OrderedDicts, plus code f… #411

Merged
merged 14 commits into from
Sep 17, 2019

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Sep 16, 2019

This PR makes model.diagnostics and model.output_writers into OrderedDicts by default.

In addition, we add functionality that preserves the existing behavior: when the user calls push!(model.diagnostics, new_diagnostic), the diagnostic new_diagnostic is given a default name (something like :diag1). In addition, the diagnostics can be accessed by index (an integer) or by name (assumed to be a Symbol, though in principle anything except an integer might be used).

Giving names to diagnostics should prove useful for saving Timeseries diagnostics, or accessing the value of a diagnostic for logging / other purposes.

We use OrderedDict so that the diagnostics and output writers are executed in the order they were inserted, which is a potentially useful feature for interdependent diagnostics.

We also add code that extends the functionality of Timeseries so that the user may pass a NamedTuple of timeseries, eg:

cfl_timeseries = Timeseries((adv=AdvectiveCFL(dt), diff=DiffusiveCFL(dt)), frequency=1)
model.diagnostics[:timeseries] = cfl_timeseries

The sampled values would then be stored in cfl_timeseries.data.adv and cfl_timeseries.data.diff. By extending getproperty, we also provide syntax such that

cfl_timeseries.adv

returns the array cfl_timeseries.data.adv, for user convenience. As for "non-tupled" diagnostics, the samples in cfl_timeseries.data.adv can be plotted alongside cfl_timeseries.time.

Resolves #362.
Resolves #361.

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #411 into master will increase coverage by 0.33%.
The diff coverage is 91.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #411      +/-   ##
==========================================
+ Coverage   72.98%   73.31%   +0.33%     
==========================================
  Files          25       25              
  Lines        1403     1428      +25     
==========================================
+ Hits         1024     1047      +23     
- Misses        379      381       +2
Impacted Files Coverage Δ
src/Oceananigans.jl 75% <ø> (ø) ⬆️
src/output_writers.jl 80.11% <0%> (-0.46%) ⬇️
src/models.jl 80.95% <100%> (ø) ⬆️
src/time_steppers.jl 74.3% <100%> (ø) ⬆️
src/utils.jl 72.81% <100%> (+3.99%) ⬆️
src/diagnostics.jl 78.33% <88.88%> (+2.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97927e2...5e6de4f. Read the comment docs.

@ali-ramadhan ali-ramadhan added the feature 🌟 Something new and shiny label Sep 17, 2019
@ali-ramadhan ali-ramadhan changed the title Put Diagnostics and OutputWriters into OrderedDicts, plus code for tupling Timeseries Put Diagnostics and OutputWriters into OrderedDicts, plus code f… Sep 17, 2019
@ali-ramadhan ali-ramadhan merged commit cf55bcd into master Sep 17, 2019
@ali-ramadhan ali-ramadhan deleted the diagnostics-refactor branch September 17, 2019 11:15
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
…iMA#411)

Put Diagnostics and OutputWriters into OrderedDicts, plus code for tupling Timeseries

Former-commit-id: cf55bcd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 Something new and shiny
Projects
None yet
2 participants