Skip to content

(0.106.4) Move NetCDF output attribute methods from extension to Models module#5469

Merged
simone-silvestri merged 8 commits intomainfrom
ss/fix-netcdf-extension
Apr 2, 2026
Merged

(0.106.4) Move NetCDF output attribute methods from extension to Models module#5469
simone-silvestri merged 8 commits intomainfrom
ss/fix-netcdf-extension

Conversation

@simone-silvestri
Copy link
Copy Markdown
Collaborator

@simone-silvestri simone-silvestri commented Apr 1, 2026

Summary

  • Moves default_output_attributes, default_velocity_attributes, and default_tracer_attributes method implementations from OceananigansNCDatasetsExt into src/Models/output_attributes.jl
  • Keeps only a generic fallback default_output_attributes(model) = Dict() in OutputWriters
  • Models defines specialized methods via dispatch on NonhydrostaticModel, HydrostaticFreeSurfaceModel, and ShallowWaterModel
  • add_schedule_metadata! methods remain in OutputWriters (no external type dependencies)

Motivation

These functions don't depend on NCDatasets but were defined entirely inside the extension. This prevented downstream package extensions (e.g. ClimaSeaIceNCDatasetsExt) from adding methods at compile time, forcing them to use __init__ + @eval which breaks incremental compilation on Julia 1.12.

Moving the methods to Models (following the existing pattern for default_included_properties and checkpointer_address) makes them available to downstream packages via standard import.

Companion PR: CliMA/ClimaSeaIce.jl — ss/fix-netcdf-extension

Test plan

  • Existing NetCDF writer tests pass
  • ClimaSeaIce precompiles successfully with NumericalEarth on Julia 1.12

…in module

These functions were defined entirely in OceananigansNCDatasetsExt despite
not depending on NCDatasets. This prevented downstream extensions (e.g.
ClimaSeaIce) from adding methods at compile time, forcing them to use
__init__ + @eval which breaks incremental compilation on Julia 1.12.

By declaring function stubs in src/OutputWriters and importing them in
the extension, any package extension can now extend these functions with
a simple `import` + method definition at compile time.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simone-silvestri added a commit to CliMA/ClimaSeaIce.jl that referenced this pull request Apr 1, 2026
Replace __init__ + @eval pattern with a direct import and compile-time
method definition for default_output_attributes. The old pattern broke
incremental compilation on Julia 1.12 because eval into a closed module
is no longer allowed.

Requires Oceananigans >= 0.106.4 (CliMA/Oceananigans.jl#5469).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment on lines +73 to +74
default_output_attributes,
add_schedule_metadata!,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to my comments in CliMA/ClimaSeaIce.jl#127 (review) (including the use of import 🥲), feels like these methods should really live in OutputWriters, not in this extension.

Copy link
Copy Markdown
Collaborator

@giordano giordano Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the fact that you tried to use @eval to circumvent the impossibility to properly define the methods is a strong indication that the methods just currently live in the wrong places (the extensions, where they have nothing specific to the extensions)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? they do live in OutputWriters; they are just extended here. What specifically do you suggest?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean put this chunk of code in OutputWriters?

#####
##### Variable attributes
#####

default_velocity_attributes(::RectilinearGrid) = Dict(
    "u" => Dict("long_name" => "Velocity in the +x-direction.", "units" => "m/s"),
    "v" => Dict("long_name" => "Velocity in the +y-direction.", "units" => "m/s"),
    "w" => Dict("long_name" => "Velocity in the +z-direction.", "units" => "m/s"))

default_velocity_attributes(::LatitudeLongitudeGrid) = Dict(
    "u"            => Dict("long_name" => "Velocity in the zonal direction (+ = east).", "units" => "m/s"),
    "v"            => Dict("long_name" => "Velocity in the meridional direction (+ = north).", "units" => "m/s"),
    "w"            => Dict("long_name" => "Velocity in the vertical direction (+ = up).", "units" => "m/s"),
    "displacement" => Dict("long_name" => "Sea surface height displacement", "units" => "m"))

default_velocity_attributes(ibg::ImmersedBoundaryGrid) = default_velocity_attributes(ibg.underlying_grid)

default_tracer_attributes(::Nothing) = Dict()

default_tracer_attributes(::BuoyancyForce{<:BuoyancyTracer}) = Dict("b" => Dict("long_name" => "Buoyancy", "units" => "m/s²"))

default_tracer_attributes(::BuoyancyForce{<:SeawaterBuoyancy{FT, <:LinearEquationOfState}}) where FT = Dict(
    "T" => Dict("long_name" => "Temperature", "units" => "°C"),
    "S" => Dict("long_name" => "Salinity",    "units" => "practical salinity unit (psu)"))

default_tracer_attributes(::BuoyancyBoussinesqEOSModel) = Dict("T" => Dict("long_name" => "Conservative temperature", "units" => "°C"),
                                                               "S" => Dict("long_name" => "Absolute salinity",        "units" => "g/kg"))

function default_output_attributes(model)
    velocity_attrs = default_velocity_attributes(model.grid)
    buoyancy = model isa ShallowWaterModel ? nothing : model.buoyancy
    tracer_attrs = default_tracer_attributes(buoyancy)
    return merge(velocity_attrs, tracer_attrs)
end

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and


#####
##### Saving schedule metadata as global attributes
#####

add_schedule_metadata!(attributes, schedule) = nothing

function add_schedule_metadata!(global_attributes, schedule::IterationInterval)
    global_attributes["schedule"] = "IterationInterval"
    global_attributes["interval"] = schedule.interval
    global_attributes["output iteration interval"] = "Output was saved every $(schedule.interval) iteration(s)."

    return nothing
end

function add_schedule_metadata!(global_attributes, schedule::TimeInterval)
    global_attributes["schedule"] = "TimeInterval"
    global_attributes["interval"] = schedule.interval
    global_attributes["output time interval"] = "Output was saved every $(prettytime(schedule.interval))."

    return nothing
end

function add_schedule_metadata!(global_attributes, schedule::WallTimeInterval)
    global_attributes["schedule"] = "WallTimeInterval"
    global_attributes["interval"] = schedule.interval
    global_attributes["output time interval"] =
        "Output was saved every $(prettytime(schedule.interval))."

    return nothing
end

function add_schedule_metadata!(global_attributes, schedule::AveragedTimeInterval)
    global_attributes["schedule"] = "AveragedTimeInterval"
    global_attributes["interval"] = schedule.interval
    global_attributes["output time interval"] = "Output was time-averaged and saved every $(prettytime(schedule.interval))."

    global_attributes["time_averaging_window"] = schedule.window
    global_attributes["time averaging window"] = "Output was time averaged with a window size of $(prettytime(schedule.window))"

    global_attributes["time_averaging_stride"] = schedule.stride
    global_attributes["time averaging stride"] = "Output was time averaged with a stride of $(schedule.stride) iteration(s) within the time averaging window."

    return nothing
end

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? they do live in OutputWriters; they are just extended here.

The functions are declared in OutputWriters, the methods are defined in the extensions.

What specifically do you suggest?

To have the methods in OutputWriters, not in the extensions. I don't see anything specific to NCDatasets in those methods.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you're right. there is no dependence on NCDatasets as a package; it is just that the methods are specific to NetCDF. But there is no functional barrier to putting them in OutputWriters. I can do that.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess these methods were initially needed only internally in the extension, and that's fair, but then when they grew to be needed in downstream packages having them in the extension became odd, and really not technically necessary.

glwagner and others added 2 commits April 1, 2026 17:27
…iters

These methods have no dependency on NCDatasets and belong in the main
OutputWriters module, not the NCDatasets extension. Also removes the
now-unused type aliases and imports from the extension.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
glwagner and others added 2 commits April 1, 2026 17:57
OutputWriters is included before BuoyancyFormulations and Models, so the
method definitions can't live directly in OutputWriters.jl. Instead, keep
stubs in OutputWriters and define the actual methods in a separate file
(output_attributes.jl) that is included from Oceananigans.jl after Models.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The file is included at top-level Oceananigans scope before the
using .Grids etc. statements, so it needs its own explicit imports.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@simone-silvestri
Copy link
Copy Markdown
Collaborator Author

there is the problem that OutputWriters is loaded before many other modules that these functions require. We could load OutputWriters after actually. I think functions in OutputWriters are only extended in Modules

@glwagner
Copy link
Copy Markdown
Member

glwagner commented Apr 2, 2026

there is the problem that OutputWriters is loaded before many other modules that these functions require. We could load OutputWriters after actually. I think functions in OutputWriters are only extended in Modules

We can define the functions in OutputWriters, and then add methods in the modules. That's the canonical way.

simone-silvestri and others added 2 commits April 2, 2026 12:18
…n OutputWriters

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…buoyancy

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@simone-silvestri simone-silvestri changed the title Move NetCDF attribute function stubs to main module Move NetCDF output attribute methods from extension to Models module Apr 2, 2026
@simone-silvestri simone-silvestri changed the title Move NetCDF output attribute methods from extension to Models module (0.106.4) Move NetCDF output attribute methods from extension to Models module Apr 2, 2026
@simone-silvestri simone-silvestri merged commit a7b8bed into main Apr 2, 2026
68 of 75 checks passed
@simone-silvestri simone-silvestri deleted the ss/fix-netcdf-extension branch April 2, 2026 12:54
simone-silvestri added a commit to CliMA/ClimaSeaIce.jl that referenced this pull request Apr 2, 2026
* Fix NCDatasets extension for Julia 1.12 incremental compilation

Replace __init__ + @eval pattern with a direct import and compile-time
method definition for default_output_attributes. The old pattern broke
incremental compilation on Julia 1.12 because eval into a closed module
is no longer allowed.

Requires Oceananigans >= 0.106.4 (CliMA/Oceananigans.jl#5469).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Move default_output_attributes to main module

Address review feedback: move default_output_attributes(::SeaIceModel)
and helper functions from the NCDatasets extension to the main ClimaSeaIce
module. With Oceananigans#5469, default_output_attributes is a stub in
Oceananigans.OutputWriters, so the method can be defined at compile time
without needing the NCDatasets extension. Uses qualified name instead of
import.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Remove empty NCDatasets extension

The extension has no code left after moving default_output_attributes
to the main module. Remove the extension file, weakdep, and extension
entry from Project.toml. Keep NCDatasets in [extras] for tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* retry tests

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Gregory Wagner <gregory.leclaire.wagner@gmail.com>
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