(0.96.0) Improve the NetCDFOutputWriter experience
#4046
Conversation
|
Here's what a NetCDF file looks like now in NCDatasets.jl and xarray to give an idea of the changes introduced by this PR. NCDatasets.jl: xarray: Hmmm, looks like I forgot to save some attributes for immersed boundary related variables. |
| Δxᶠᵃᵃ_name = dim_name_generator("dx", grid, f, nothing, nothing, Val(:x)) | ||
| Δxᶜᵃᵃ_name = dim_name_generator("dx", grid, c, nothing, nothing, Val(:x)) |
There was a problem hiding this comment.
It seems that dx is hardcoded here. Should we allow for more flexibility here too since this is one of the motivations behind the PR?
There was a problem hiding this comment.
Ah so I suppose dx is the "canonical" name, but the dim_name_generator can be used to override this name!
For example, a custom dim_name_generator can be passed to the NetCDFOutputWriter constructor to change dx to Δx or delta_x before passing it back to Oceananigans.OutputWriters.default_dim_name (to keep all other behavior the same).
This is totally not obvious from the code (and the docstring) so I should add a test for this and probably another docstring example. It's how I was thinking we can satisfy desired naming conventions.
But happy to switch to another better design!
There was a problem hiding this comment.
Yeah I figured that somewhere down in between this and the last comment haha. There's a comment somewhere about it. I think the way you're doing is great!
| function gather_immersed_boundary(grid::GFBIBG, indices, dim_name_generator) | ||
| op_mask_ccc = KernelFunctionOperation{Center, Center, Center}(peripheral_node, grid, Center(), Center(), Center()) | ||
| op_mask_fcc = KernelFunctionOperation{Face, Center, Center}(peripheral_node, grid, Face(), Center(), Center()) | ||
| op_mask_cfc = KernelFunctionOperation{Center, Face, Center}(peripheral_node, grid, Center(), Face(), Center()) | ||
| op_mask_ccf = KernelFunctionOperation{Center, Center, Face}(peripheral_node, grid, Center(), Center(), Face()) | ||
|
|
||
| return Dict( | ||
| "bottom_height" => Field(grid.immersed_boundary.bottom_height; indices), | ||
| "immersed_boundary_mask_ccc" => Field(op_mask_ccc; indices), | ||
| "immersed_boundary_mask_fcc" => Field(op_mask_fcc; indices), | ||
| "immersed_boundary_mask_cfc" => Field(op_mask_cfc; indices), | ||
| "immersed_boundary_mask_ccf" => Field(op_mask_ccf; indices) | ||
| ) | ||
| end |
There was a problem hiding this comment.
This function is assuming a GridFittedBottom, but it would be nice to extend it to GridFittedBoundary (the only difference is that GridFittedBoundary doesn't necessary have a bottom height).
There was a problem hiding this comment.
Yes! This function only dispatches on GridFittedBottom (via GFBIBG) but I need to also define gather_immersed_boundary for PartialCellBottom and GridFittedBoundary.
| xᶠᵃᵃ_name = dim_name_generator("x", grid, f, nothing, nothing, Val(:x)) | ||
| xᶜᵃᵃ_name = dim_name_generator("x", grid, c, nothing, nothing, Val(:x)) | ||
| yᵃᶠᵃ_name = dim_name_generator("y", grid, nothing, f, nothing, Val(:y)) | ||
| yᵃᶜᵃ_name = dim_name_generator("y", grid, nothing, c, nothing, Val(:y)) | ||
|
|
||
| Δxᶠᵃᵃ_name = dim_name_generator("dx", grid, f, nothing, nothing, Val(:x)) | ||
| Δxᶜᵃᵃ_name = dim_name_generator("dx", grid, c, nothing, nothing, Val(:x)) | ||
| Δyᵃᶠᵃ_name = dim_name_generator("dy", grid, nothing, f, nothing, Val(:y)) | ||
| Δyᵃᶜᵃ_name = dim_name_generator("dy", grid, nothing, c, nothing, Val(:y)) |
There was a problem hiding this comment.
Same comment as before. Should we avoid hardcoding these?
| xᶠᵃᵃ_attrs = Dict("long_name" => "Locations of the cell faces in the x-direction.", "units" => "m") | ||
| xᶜᵃᵃ_attrs = Dict("long_name" => "Locations of the cell centers in the x-direction.", "units" => "m") | ||
| yᵃᶠᵃ_attrs = Dict("long_name" => "Locations of the cell faces in the y-direction.", "units" => "m") | ||
| yᵃᶜᵃ_attrs = Dict("long_name" => "Locations of the cell centers in the y-direction.", "units" => "m") |
There was a problem hiding this comment.
| xᶠᵃᵃ_attrs = Dict("long_name" => "Locations of the cell faces in the x-direction.", "units" => "m") | |
| xᶜᵃᵃ_attrs = Dict("long_name" => "Locations of the cell centers in the x-direction.", "units" => "m") | |
| yᵃᶠᵃ_attrs = Dict("long_name" => "Locations of the cell faces in the y-direction.", "units" => "m") | |
| yᵃᶜᵃ_attrs = Dict("long_name" => "Locations of the cell centers in the y-direction.", "units" => "m") | |
| xᶠᵃᵃ_attrs = Dict("long_name" => "Cell face locations in the x-direction.", "units" => "m") | |
| xᶜᵃᵃ_attrs = Dict("long_name" => "Cell center locations in the x-direction.", "units" => "m") | |
| yᵃᶠᵃ_attrs = Dict("long_name" => "Cell face locations in the y-direction.", "units" => "m") | |
| yᵃᶜᵃ_attrs = Dict("long_name" => "Cell center locations in the y-direction.", "units" => "m") |
I always thought these names were pretty long and not super easy to read as axis labels in figures. I made them a bit shorter but I wonder if we can do better. Perhaps x node locations (faces) on so on? That way x is the first thing in the description, making it easier for the user/reader/viewer.
There was a problem hiding this comment.
Also we have always assumed SI units for the outputs. I wonder if it would be hard to include a flag to not do that.
| λᶠᵃᵃ_attrs = Dict("long_name" => "Locations of the cell faces in the zonal direction.", "units" => "degrees east") | ||
| λᶜᵃᵃ_attrs = Dict("long_name" => "Locations of the cell centers in the zonal direction.", "units" => "degrees east") |
There was a problem hiding this comment.
Same comment here for the names and units.
There was a problem hiding this comment.
Ah, I now understand that these are just the default ones! Somewhere down there must be an option to provide user-defined ones. Got it! Disregard the other comments about this,
| immersed_attrs = Dict( | ||
| "immersed_boundary_type" => string(nameof(typeof(ibg.immersed_boundary))) | ||
| ) | ||
|
|
| deflatelevel = 0, | ||
| part = 1, | ||
| file_splitting = NoFileSplitting(), | ||
| verbose = false) | ||
| dimension_name_generator = default_dim_name) |
There was a problem hiding this comment.
This is a great way to do things. Maybe in the future we can include functions that already follow a given popular/useful convention. I think we should probably also provide one that uses the exact names Oceananigans uses (I assume that'll be needed for the integration with FieldTimeSeries?)
| outputs = Dict( | ||
| string(name) => construct_output(outputs[name], grid, indices, with_halos) | ||
| for name in keys(outputs) | ||
| ) |
There was a problem hiding this comment.
Probably we should follow the conventions we already use in Oceananigans?
| outputs = Dict( | |
| string(name) => construct_output(outputs[name], grid, indices, with_halos) | |
| for name in keys(outputs) | |
| ) | |
| outputs = Dict(string(name) => construct_output(outputs[name], grid, indices, with_halos) | |
| for name in keys(outputs) |
Or in this case it might be better to even do a one-liner.
| define_output_variable!( | ||
| dataset, | ||
| materialized, | ||
| name, | ||
| array_type, | ||
| deflatelevel, | ||
| attributes, | ||
| dimensions, | ||
| filepath, # for better error messages | ||
| dimension_name_generator, | ||
| false # time_dependent = false | ||
| ) |
There was a problem hiding this comment.
Again, maybe we should keep consistency with the rest of Oceananigans formatting.
| define_output_variable!( | |
| dataset, | |
| materialized, | |
| name, | |
| array_type, | |
| deflatelevel, | |
| attributes, | |
| dimensions, | |
| filepath, # for better error messages | |
| dimension_name_generator, | |
| false # time_dependent = false | |
| ) | |
| define_output_variable!(dataset, | |
| materialized, | |
| name, | |
| array_type, | |
| deflatelevel, | |
| attributes, | |
| dimensions, | |
| filepath, # for better error messages | |
| dimension_name_generator, | |
| false # time_dependent = false) |
There was a problem hiding this comment.
Agree with this comment generally, we can start a discussion to format the code differently throughout, but either way I think we should strive to format consistently.
|
I haven't checked the tests yet, but so far it's looking great! I'll try to check the tests tomorrow. |
It's not a ReducedField, its a WindowedField. But properly supporting WindowedField is intrinsic to NetCDF output writing without halos so there should be a general solution to this. |
I'm not sure a FieldTimeSeries refactor is necessary (note that we also support NetCDF FieldTimeSeries on ClimaOcean, so I don't entirely understand why refactoring is necessary). However, in the case that such a refactor bumps the version again, would it be prudent to be more conservative about version bumping here? |
This PR updates the
NetCDFOutputWriterto:RectilinearGridandLatitudeLongitudeGrid(with correct and useful output attributes).LagrangianParticlesoutput.FieldTimeSeriesconstruction from NetCDF (full support to be added in a subsequent PR).Tests have been added for all these features. Thank you to @tomchor and @almacarolina for helpful discussions during this refactor!
There's still a little bit left to do, but if anyone has any thoughts or feedback and would like to leave a review I'd appreciate it! There are a lot of line additions but thankfully the changes are almost fully isolated to just two files.
TODO:
xareas,volumes, etc. viaKernelFunctionOperation?NetCDFOutputWriterto highlight all features.NetCDFOutputWriter+ some fancier features in an example.Some comments:
FieldTimeSeries. I was planning on working on it here (as the branch name suggests) but it will involve refactoringfield_time_series.jlwhich seems best suited for a separate PR.Fieldbut is kind of aReducedFieldwe cannot dispatch on its type and work with it correctly. Will open an issue to discuss.NetCDFOutputWriter.Resolves #1334 (via the dimension name generator)
Resolves #2248
Resolves #2770 (Maybe? You can save u, v, w, T, S, and η in the same NetCDF file but it's a bit hacky, see above.)
Resolves #3997
This PR makes progress on issue #3935
This PR supercedes PR #2652