Time averaging and FieldSlicer for NetCDFOutputWriter
#1040
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1040 +/- ##
===========================================
- Coverage 54.75% 24.44% -30.31%
===========================================
Files 160 158 -2
Lines 3866 3681 -185
===========================================
- Hits 2117 900 -1217
- Misses 1749 2781 +1032
Continue to review full report at Codecov.
|
| previous :: Float64 | ||
| verbose :: Bool | ||
| end | ||
|
|
||
| """ | ||
| NetCDFOutputWriter(model, outputs; filename, iteration_interval=nothing, time_interval=nothing, | ||
| NetCDFOutputWriter(model, outputs; filepath, iteration_interval=nothing, time_interval=nothing, |
There was a problem hiding this comment.
In JLD2OutputWriter we have two arguments: dir, and filename. This is combined internally into a filepath. I suppose the reasoning is that its common to have multiple output writers (which you'd want to save data into the same directory), but different names. Having two keyword arguments makes that slightly more convenient. I'm ok with using filepath instead if there's a good argument. But I think both output writers should have the same interface?
There was a problem hiding this comment.
Actually, looks like JLD2OutputWriter uses "prefix":
The distinction is that prefix is appended with various endings (eg .jld2, or sometimes _part$n.jld2) to complete the file name.
Happy to change JLD2OutputWriter to conform to a style we think is clearest --- let's discuss.
There was a problem hiding this comment.
I actually think a friendly interface would use filename, but allow the user to give only the first "part" of the file name, leaving out .jld2 or .nc --- which are already made obvious by the name of the object (NetCDF* or JLD2*). This has the advantage that this keyword argument doesn't need to be changed if switching between output writers.
There was a problem hiding this comment.
Yeah I agree a shared interface would be good, but I think the reason JLD2OutputWriter has both a dir and filename is to support file splitting with automatic file naming.
The filepath should include both the directory and filename so I think having two arguments is kind of clunky. Ideally both would just use a filepath.
I guess NetCDFOutputWriter gets away with just a filepath because it doesn't support file splitting so the filename never changes during the course of a simulation.
We could modify the JLD2OutputWriter to just take in a filepath. Two possible solutions are:
- File splitting could be supported by injecting
part1,part2, etc. appropriately into thefilepath, or - via another kwarg
filename_patternthat's a functionfilename_pattern(n::Int)::Stringthat returns the filename for partn.
There was a problem hiding this comment.
A third alternative would be to remove the ability to split files, but I think this is a bad idea. While there is no file size limit for HDF5 and we typically don't split files since our files aren't huge, there will be users in the future who will run huge models and need this functionality due to memory limits (or even filesystem limits).
There was a problem hiding this comment.
I guess NetCDFOutputWriter gets away with just a filepath because it doesn't support file splitting so the filename never changes during the course of a simulation.
Do we plan on supporting file splitting for NetCDFOutputWriter in the future?
There was a problem hiding this comment.
Yeah I think we should. We should continue discussion in #884 (where it seems we already liked filepath haha).
|
Does |
Yes good point. I'll nuke them and probably just need to do some |
This PR refactors the
NetCDFOutputWriterto useFieldSlicer(which cleans it up a bit) and adds support for time-averaging for NetCDF. All the existing NetCDF tests pass (of which there are quite a few).I also added a test for strided windowed time averaging of horizontal averages for
NetCDFOutputWriter. Oceananigans solves∂c/∂t = - λ(x, y, z) cwhereλ(x, y, z) = x + (1 - y)^2 + tanh(z)which is independent exoponential decay at every grid point so you can analytically compute what the output of the horizontal average and the strided windowed time average should be. Thankfully the test passes 🎉I also reorganized
test_output_writers.jlquite a bit. I think it's big enough that it should be split up into multiple files but I'll leave this for a future PR since it would make reviewing this PR's diff difficult.Would be nice if NetCDF accepted a named tuple for
outputsand had a less clunky interface than just dicts for everything. Might have to wait for a future PR though...X-Ref: Alexander-Barth/NCDatasets.jl#105
Resolves #876