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.

Sign up for GitHub

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

Jump to bottom

(0.96.0) Improve the NetCDFOutputWriter experience #4046

Open
wants to merge 54 commits into
base: main
Choose a base branch
from
Open

(0.96.0) Improve the NetCDFOutputWriter experience #4046

wants to merge 54 commits into from
+3,328 −805

Conversation

ali-ramadhan
Copy link
Member

@ali-ramadhan ali-ramadhan commented Jan 16, 2025 •
edited
Loading

This PR updates the NetCDFOutputWriter to:

  • Properly work on RectilinearGrid and LatitudeLongitudeGrid (with correct and useful output attributes).
  • Save grid metrics with useful attributes.
  • Save immersed boundary information.
  • Properly support flat grids.
  • Work cleanly with LagrangianParticles output.
  • Allow for flexible dimension naming to satisfy desired naming conventions.
  • Write grid reconstruction metadata into a NetCDF group to support FieldTimeSeries construction 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:

  • Output grid areas and volumes too? But maybe it would be nice to first have xareas, volumes, etc. via KernelFunctionOperation?
  • Further improve the docstring and documentation for NetCDFOutputWriter to highlight all features.
  • Use NetCDFOutputWriter + some fancier features in an example.
  • Figure out which tests are missing.
  • Watch out for failing tests/examples and fix them.

Some comments:

  • I'm hoping to merge this PR and immediately work on adding support for NetCDF-backed FieldTimeSeries. I was planning on working on it here (as the branch name suggests) but it will involve refactoring field_time_series.jl which seems best suited for a separate PR.
  • Supporting output of the free surface height is a bit hacky. Since it's a regular Field but is kind of a ReducedField we cannot dispatch on its type and work with it correctly. Will open an issue to discuss.
  • I'm tagging v0.96.0 since this PR significantly changes the dimension names used in NetCDF files produced by 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

@ali-ramadhan
Start refactoring NetCDFOutputWriter to make it more flexible
0bcc0f5
@ali-ramadhan
Refactor and update NetCDFOutputWriter tests
b445f0c
@ali-ramadhan
Fix tests
adeadc4
@ali-ramadhan
Test and fix NetCDF + Lagrangian particles
e63335b
@ali-ramadhan
Better file names and file management for NetCDF tests
daa2ace
@ali-ramadhan
Reorganize netcdf_output_writer.jl
869f418
@ali-ramadhan
Integrate dimension_name_generator into NetCDFOutputWriter
28011c3
@ali-ramadhan
Add option to output grid metrics to NetCDF
6794267
@ali-ramadhan
Correct float type for time output
cedef31
@ali-ramadhan
Update and fix tests
13907c6
@ali-ramadhan
Fix typos in rectilinear grid metric outputs
4a5a875
@ali-ramadhan
Add attributes to rectilinear grid metrics
8f2b009
@ali-ramadhan
Fix output construction for grid metrics
be9b1c3
@ali-ramadhan
Add big test for NetCDF grid metrics on rectilinear grid
1838122
@ali-ramadhan
Fix and test NetCDF sliced output with grid metrics
dbf1c61
@ali-ramadhan
Time should always be Float64
78ceb70
@ali-ramadhan
Forgot to close and remove some files
b86b032
@ali-ramadhan
Fixes to support NetCDF output for flat grids
3bab6f4
@ali-ramadhan
Testing NetCDF output with flat xy grids
e408cc0
@ali-ramadhan
Fixes to support NetCDF output for flat grids
bc0f450
@ali-ramadhan
Test xz and yz flat grids
84574b8
@ali-ramadhan
Fix time eltype tests
1b7c449
@ali-ramadhan
Test NetCDF output for 1D column grids
32279cd
@ali-ramadhan
Output the immersed boundary with NetCDF
295ea11
@ali-ramadhan
Test immersed boundaries with flat xz and yz grids
ec62a77
@ali-ramadhan
Support NetCDF output for lat-lon grids
d3b975a
@ali-ramadhan
Test NetCDF output of lat-lon grid metrics
5106dbb
@ali-ramadhan
Test NetCDF output of bathymetry with GridFittedBottom
ec80937
@ali-ramadhan
Typo
0a7a19f
@ali-ramadhan
Update default output attributes to be more correct
1fc0b1a
@tomchor
Copy link
Collaborator

tomchor commented Jan 17, 2025

I haven't checked the tests yet, but so far it's looking great! I'll try to check the tests tomorrow.

@glwagner
Copy link
Member

Supporting output of the free surface height is a bit hacky. Since it's a regular Field but is kind of a ReducedField we cannot dispatch on its type and work with it correctly. Will open an issue to discuss.

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.

@glwagner
Copy link
Member

I'm tagging v0.96.0 since this PR significantly changes the dimension names used in NetCDF files produced by NetCDFOutputWriter.

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?

@glwagner
Copy link
Member

Supporting output of the free surface height is a bit hacky. Since it's a regular Field but is kind of a ReducedField we cannot dispatch on its type and work with it correctly. Will open an issue to discuss.

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.

In case its helpful: a WindowedField is a Field with non-default indices. We omit halo regions by producing a windowed field with indices between (1:Nx, 1:Ny, 1:Nz) (for example), rather than (:, :, :) which span the entire domain including halos. The free surface has indices (:, :, Nz+1) --- indicating that it is not a reduced field with physical validity throughout the domain at z, but rather a Field that occupies a specific slice of the domain (k=Nz+1).

Thus with a general way of "windowing" dimensions, we should support free surface and with_halos=false. I'm not sure how the latter is supported without supporting the free surface, though.

@ali-ramadhan
Copy link
Member Author

@glwagner Thanks for pointing out that the free surface is a WindowedField. I should be able to add proper/cleaner support now!

@ali-ramadhan
Copy link
Member Author

I'm tagging v0.96.0 since this PR significantly changes the dimension names used in NetCDF files produced by NetCDFOutputWriter.

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?

Ah I may have just assumed a large refactor would be necessary without digging into FieldTimeSeries first!

I believe that according to semantic versioning which Oceananigans.jl follows (still I think?), this PR would necessitate a minor version bump as it changes the NetCDFOutputWriter interface and any code that interfaces with the NetCDF output files will break.

It's probably not nice to increment the minor version very often, but is there a particular reason to be conservative about version bumping?

@glwagner
Copy link
Member

glwagner commented Mar 3, 2025 •
edited
Loading

I'm tagging v0.96.0 since this PR significantly changes the dimension names used in NetCDF files produced by NetCDFOutputWriter.

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?

Ah I may have just assumed a large refactor would be necessary without digging into FieldTimeSeries first!

I believe that according to semantic versioning which Oceananigans.jl follows (still I think?), this PR would necessitate a minor version bump as it changes the NetCDFOutputWriter interface and any code that interfaces with the NetCDF output files will break.

It's probably not nice to increment the minor version very often, but is there a particular reason to be conservative about version bumping?

For sure if introduces a breaking change to NetCDFOutputWriter then we need a new minor version.

Re: conservatism, no, I think we just want to try our best to follow the protocol; ie do not bump when not warranted, so as not to confuse interpretation of code changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomchor tomchor tomchor left review comments

@glwagner glwagner glwagner left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
output 💾
Projects
None yet
Milestone
No milestone
3 participants
@ali-ramadhan @tomchor @glwagner