(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. |
| 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ᶠᵃᵃ_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? |
| return nothing | ||
| end | ||
|
|
||
| function test_netcdf_rectilinear_column(arch) |
There was a problem hiding this comment.
I see that the tests are pretty thorough! Well done!
I guess my only comment is that it seems we're re-writing a bunch of code. For example, test_netcdf_rectilinear_flat_yz() is pretty much the same thing as their xy and xz counterparts. I wonder if it would be better to try and re-use much of that code, so that it's easier to maintain.
On the other hand, re-using code like this can make way less readable, so I'm torn. I just wanted to comment this so that we think at least think about it.
@ali-ramadhan what are your thoughts?
There was a problem hiding this comment.
Also it seems that specifically test_netcdf_rectilinear_column() is the only topology where we don't test all three directions...
In case its helpful: a WindowedField is a Field with non-default Thus with a general way of "windowing" dimensions, we should support free surface and |
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