Add write_to_netcdf support for OutputVar#370
Conversation
ph-kev
left a comment
There was a problem hiding this comment.
Thank you for working on this issue! I left some comments that should be addressed. Also, could you also add this to NEWS.md as well? Let me know if there was anything that isn't clear.
| # # Test 7: Scalar (0D) round-trip | ||
| # scalar_path = joinpath(tmpdir, "scalar_file.nc") |
There was a problem hiding this comment.
Is this not possible in general with NetCDF files?
There was a problem hiding this comment.
Scalar (0D) variables are supported in NetCDF. The CF Conventions explicitly describe “scalar coordinate variables” as variables with no dimensions (see CF Conventions section 5.7: https://cfconventions.org/Data/cf-conventions/cf-conventions-1.11/cf-conventions.html#scalar-coordinate-variables)
NCDatasets.jl can also read and write scalar variables, so the limitation here is just that scalar NetCDF I/O hasn’t been wired up and tested in ClimaAnalysis yet, which is why this test is currently commented out. I’m happy to either enable and fix the scalar round-trip test in this PR or open a follow-up issue specifically for scalar support in write_to_netcdf / read_var, depending on what you prefer.
| v isa AbstractString && return v | ||
| v isa Number && return v | ||
| v isa AbstractArray && eltype(v) <: Union{Number, AbstractString} && | ||
| return v |
There was a problem hiding this comment.
There are different concrete types of AbstractString like String, LazyString, and SubString. Even though, we probably won't expect any of these, I think it would be better to do
v isa AbstractString && string(v)I think you can apply a similar reasoning to the other paths of the function as well.
There was a problem hiding this comment.
I’ve updated _netcdf_safe_attrib to convert AbstractString values to concrete String via string(v) for safety, as you suggested. For arrays of strings, I now also convert AbstractString[] to String[], so cases like SubString or LazyString are handled before serialization.
For numeric attributes, I kept them as-is since NCDatasets.jl already supports the usual Julia numeric types ( Int, Float32, Float64), and anything more exotic will fall back to the final string(v) case. Let me know if you’d prefer stricter conversion there as well.
Purpose
closes #368
This PR adds support for writing an
OutputVarback to a NetCDF file viawrite_to_netcdf.The goal is to enable round trip workflow where users can read data, perform analysis, and then persist the resulting variable for later use.
To-do
Content
write_to_netcdf(path, var)to write anOutputVarto a NetCDF file, including its data, dimensions, and associated attributes._netcdf_safe_attribto convert attribute values to NetCDF-compatible types when writing.
short_namerequirement for NetCDF output.Notes on scalar variables
Scalar (0D) variables: A round-trip test for scalar
OutputVars (Test 7 intest/test_Var.jl) is currently commented out.When reading a scalar variable from NetCDF,
read_varreturns an empty, untypedOrderedDictfor dimensions. This does not match the typed dictionary expected by theOutputVarconstructor and results in aMethodErrorduring the read step.Since this behavior originates in the existing read/constructor path and not in the new NetCDF writing logic, it was left unchanged here to keep this PR focused on issue #368. This can be addressed separately in a follow-up PR if desired.
If there are any adjustments you’d like to see I’m happy to make those changes.