Skip to content

Add write_to_netcdf support for OutputVar#370

Open
AmanKashyap0807 wants to merge 1 commit into
CliMA:mainfrom
AmanKashyap0807:main
Open

Add write_to_netcdf support for OutputVar#370
AmanKashyap0807 wants to merge 1 commit into
CliMA:mainfrom
AmanKashyap0807:main

Conversation

@AmanKashyap0807
Copy link
Copy Markdown

Purpose

closes #368

This PR adds support for writing an OutputVar back to a NetCDF file via write_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

  • Implement OutputVar to NetCDF file
  • Add tests for NetCDF write
  • Document the new functionality

Content

  • Added write_to_netcdf(path, var) to write an OutputVar to a NetCDF file, including its data, dimensions, and associated attributes.
  • Introduced _netcdf_safe_attrib to convert attribute values to NetCDF-
    compatible types when writing.
  • Added basic validation to ensure dimension consistency and short_name requirement for NetCDF output.
  • Added tests covering I/O, replacement of existing files, preservation of numeric types , handling of NaN .
  • updated the documentation.

Notes on scalar variables

Scalar (0D) variables: A round-trip test for scalar OutputVars (Test 7 in test/test_Var.jl) is currently commented out.
When reading a scalar variable from NetCDF, read_var returns an empty, untyped OrderedDict for dimensions. This does not match the typed dictionary expected by the OutputVar constructor and results in a MethodError during 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.


  • I have read and checked the items on the review checklist.

Copy link
Copy Markdown
Member

@ph-kev ph-kev left a comment

Choose a reason for hiding this comment

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

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.

Comment thread src/Var.jl
Comment thread src/Var.jl
Comment thread src/Var.jl
Comment thread test/test_Var.jl
Comment thread test/test_Var.jl
Comment thread src/Var.jl
Comment thread test/test_Var.jl
Comment thread test/test_Var.jl
Comment on lines +3724 to +3725
# # Test 7: Scalar (0D) round-trip
# scalar_path = joinpath(tmpdir, "scalar_file.nc")
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.

Is this not possible in general with NetCDF files?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread test/test_Var.jl
Comment thread src/Var.jl
Comment on lines +555 to +558
v isa AbstractString && return v
v isa Number && return v
v isa AbstractArray && eltype(v) <: Union{Number, AbstractString} &&
return v
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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

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.

Add functionality to write an OutputVar to a netcdf file

2 participants