Skip to content

Support NetCDF output on any AbstractVerticalCoordinate#5689

Open
ewquon wants to merge 4 commits into
mainfrom
eq/netcdf-abstract-vertical-coordinate
Open

Support NetCDF output on any AbstractVerticalCoordinate#5689
ewquon wants to merge 4 commits into
mainfrom
eq/netcdf-abstract-vertical-coordinate

Conversation

@ewquon

@ewquon ewquon commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

NetCDFWriter errored with a MethodError on any grid whose vertical coordinate is an AbstractVerticalCoordinate other than the two concrete built-ins. The NCDatasets extension dispatched vertical_coordinate_name, gather_vertical_dimensions, and default_vertical_dimension_attributes only on StaticVerticalDiscretization and MutableVerticalDiscretization, so a third subtype matched no method and died at writer construction.

This came up with a terrain-following σ coordinate defined downstream — Breeze.jl's TerrainFollowingVerticalDiscretization, which <: AbstractVerticalCoordinate — but the fix is generic.

Change

  • Widen the two non-static methods (gather_vertical_dimensions, default_vertical_dimension_attributes) from ::MutableVerticalDiscretization to ::AbstractVerticalCoordinate.
  • Add a vertical_coordinate_name(::AbstractVerticalCoordinate) = "r" fallback.

Such coordinates save their reference coordinate r (read from the shared cᵃᵃᶠ/cᵃᵃᶜ fields); physical height is reconstructed at read time from r and the coordinate transform — identical to the existing z-star path.

Why it's safe

  • MutableVerticalDiscretization behavior is unchanged: MVD <: AbstractVerticalCoordinate and the method bodies are byte-identical, so MVD now simply arrives via the supertype.
  • StaticVerticalDiscretization keeps its own, more specific method (still writes physical z).
  • The per-step η/σ reconstruction output stays gated on MutableVerticalDiscretization, so a static-terrain coordinate correctly skips it.

Testing

  • New unit test (test_netcdf_abstract_vertical_coordinate_name): a custom non-MVD AbstractVerticalCoordinate routes to the "r" fallback, while Static"z" and MVD"r" still hold.
  • The existing test_netcdf_rectilinear_mvd_output (z-star) now exercises the widened ::AbstractVerticalCoordinate methods end-to-end (regression for the r write path).
  • Manually verified that NetCDFWriter constructs and writes on a downstream terrain-following grid: reference r dimension + staggered fields present.

🤖 Generated with Claude Code

ewquon and others added 2 commits June 15, 2026 16:39
The NCDatasets extension dispatched `vertical_coordinate_name`,
`gather_vertical_dimensions`, and `default_vertical_dimension_attributes` only on
the concrete `StaticVerticalDiscretization` and `MutableVerticalDiscretization`, so
`NetCDFWriter` threw a `MethodError` on a grid whose vertical coordinate is any other
`AbstractVerticalCoordinate` subtype (e.g. one defined in a downstream package).

Widen the two non-static methods to dispatch on `AbstractVerticalCoordinate` and add
a `vertical_coordinate_name(::AbstractVerticalCoordinate) = "r"` fallback. Such
coordinates save their reference (Lagrangian) coordinate `r` (the shared `cᵃᵃᶠ`/`cᵃᵃᶜ`
fields); physical height is reconstructed at read time from `r` and the coordinate
transform, identical to the existing z-star path. `Static` keeps its own method, and
`MutableVerticalDiscretization` behaviour is unchanged (`MVD <: AbstractVerticalCoordinate`).

Verified by writing a NetCDF file from a `NetCDFWriter` on a grid whose vertical
coordinate is a downstream `AbstractVerticalCoordinate` subtype.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a unit test that a custom non-static `AbstractVerticalCoordinate` subtype
(standing in for one defined in a downstream package) routes to the generic `r`
fallback in `vertical_coordinate_name`, while `StaticVerticalDiscretization` still
maps to `z` and `MutableVerticalDiscretization` still maps to `r`. The `r` NetCDF
write path itself is covered end-to-end by `test_netcdf_rectilinear_mvd_output`,
which now dispatches through the widened `::AbstractVerticalCoordinate` methods.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@glwagner

Copy link
Copy Markdown
Member

MutableVerticalDiscretization behaviour is unchanged

🇬🇧

Such coordinates save their reference (Lagrangian) coordinate r

just a correction here, the reference is not always Lagrangian

@glwagner glwagner requested a review from tomchor June 16, 2026 00:11
Per review: the reference coordinate `r` is only Lagrangian for z-star. A general
`AbstractVerticalCoordinate` (e.g. a terrain-following σ coordinate) uses `r` as a
fixed reference/computational coordinate. Drop "(Lagrangian)" from the now-generic
comments and the NetCDF `long_name` attributes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ewquon

ewquon commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator Author

Good catch — fixed in bbbfdd7. Dropped "(Lagrangian)" from the now-generic comments and the NetCDF long_name attributes (and the PR description): r is only the Lagrangian coordinate for z-star, whereas a terrain-following σ uses it as a fixed reference/computational coordinate. Also Americanized the "behaviour" 🇺🇸

@tomchor

tomchor commented Jun 16, 2026

Copy link
Copy Markdown
Member

This came up with a terrain-following σ coordinate defined downstream — Breeze.jl's TerrainFollowingVerticalDiscretization, which <: AbstractVerticalCoordinate — but the fix is generic.

I'll review this PR in just a second, but here's a question: why is TerrainFollowingVerticalDiscretization over on Breeze? I know many people who would love to have to run ocean simulations using these coordinates in Ocenanigans. Most of them are coming from ROMS/CROCO.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants