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.

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

Fixes writing of 3d-reduced Fields to NetCDF #2865

Merged
merged 13 commits into from
Feb 8, 2023
4 changes: 2 additions & 2 deletions Project.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
name = "Oceananigans"
uuid = "9e8cae18-63c1-5223-a75c-80ca9d6e9a09"
authors = ["Climate Modeling Alliance and contributors"]
version = "0.79.2"
version = "0.79.3"

[deps]
AMGX = "c963dde9-0319-47f5-bf0c-b07d3c80ffa6"
Expand Down Expand Up @@ -39,9 +39,9 @@ StructArrays = "09ab397b-f2b6-538f-b94a-2f83cf4a842a"
Tullio = "bc48ee85-29a4-5162-ae0b-a64e1601d4bc"

[compat]
AMGX = "0.1.3"
Adapt = "3"
AlgebraicMultigrid = "0.5"
AMGX = "0.1.3"
CUDA = "3.8, 3.9"
CUDAKernels = "0.4.7"
Crayons = "4"
Expand Down
2 changes: 1 addition & 1 deletion src/OutputWriters/netcdf_output_writer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ function save_output!(ds, output, model, ow, time_index, name)
data = fetch_and_convert_output(output, model, ow)
data = drop_output_dims(output, data)
colons = Tuple(Colon() for _ in 1:ndims(data))
ds[name][colons..., time_index] = data
ds[name][colons..., time_index:time_index] = data
return nothing
end

Expand Down
14 changes: 7 additions & 7 deletions test/test_computed_field.jl
Original file line number Diff line number Diff line change
Expand Up @@ -390,8 +390,8 @@ for arch in archs
end

ϵ(x, y, z) = 2rand() - 1
u, v, w = model.velocities
set!(model, u=ϵ, v=ϵ)
u, v, w = model.velocities
ζ_op = KernelFunctionOperation{Face, Face, Center}(ζ₃ᶠᶠᶜ, grid, computed_dependencies=(u, v))

ζ = Field(ζ_op) # identical to `VerticalVorticityField`
Expand All @@ -400,15 +400,15 @@ for arch in archs

ζxy = Field(ζ_op, indices=(:, :, 1))
compute!(ζxy)
@test ζxy == view(ζ, :, :, 1)
@test all(interior(ζxy, :, :, 1) .== interior(ζ, :, :, 1))

ζxz = Field(ζ_op, indices=(:, 1, :))
compute!(ζxz)
@test ζxz == view(ζ, :, 1, :)
@test all(interior(ζxz, :, 1, :) .== interior(ζ, :, 1, :))

ζyz = Field(ζ_op, indices=(1, :, :))
compute!(ζyz)
@test ζyz == view(ζ, 1, :, :)
@test all(interior(ζyz, 1, :, :) .== interior(ζ, 1, :, :))
end

@testset "Operations with computed Fields [$A, $G]" begin
Expand All @@ -417,7 +417,7 @@ for arch in archs
end

@testset "Horizontal averages of operations [$A, $G]" begin
@info " Testing horizontal averges..."
@info " Testing horizontal averages..."
@test horizontal_average_of_plus(model)
@test horizontal_average_of_minus(model)
@test horizontal_average_of_times(model)
Expand All @@ -427,12 +427,12 @@ for arch in archs
end

@testset "Zonal averages of operations [$A, $G]" begin
@info " Testing zonal averges..."
@info " Testing zonal averages..."
@test zonal_average_of_plus(model)
end

@testset "Volume averages of operations [$A, $G]" begin
@info " Testing volume averges..."
@info " Testing volume averages..."
@test volume_average_of_times(model)
end

Expand Down
46 changes: 45 additions & 1 deletion test/test_netcdf_output_writer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -504,10 +504,53 @@ function test_netcdf_function_output(arch)
return nothing
end

function test_netcdf_spatial_average(arch)
topo = (Periodic, Periodic, Periodic)
domain = (x=(0, 1), y=(0, 1), z=(0, 1))
grid = RectilinearGrid(arch, topology=topo, size=(4, 4, 4); domain...)

model = NonhydrostaticModel(grid = grid,
timestepper = :RungeKutta3,
tracers = (:c,),
coriolis = nothing,
buoyancy = nothing,
closure = nothing)
set!(model, c=1)

Δt = 1/64 # Nice floating-point number
simulation = Simulation(model, Δt=Δt, stop_time=50Δt)
Copy link
Member

@glwagner glwagner Feb 8, 2023

Choose a reason for hiding this comment

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

Can we do fewer steps? Our tests currently stretch the limits of our resources so we need to be as parsimonious as possible when adding new tests. Note also that compilation cost is the main thing. If this test can be combined with another test, that'd be ideal. For example, many different NetCDF tests could use the same simulation --- there's no need to run independent simulations?

Copy link
Member

Choose a reason for hiding this comment

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

Also suggest using stop_iteration

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry @glwagner I ended up merging before you commented. I copied the test template for others in the same file so there are more tests that we apply this change to. Would you like me to open another PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

Right, I'm pointing out that we don't want to copy/paste test code now without care, since a lot of our test code is poorly written / wasteful and our CI is straining under the pressure... :-/

Copy link
Member

Choose a reason for hiding this comment

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

Any PRs that reduce test cost will be greatly appreciated! I can't tell if all the changes will make a big difference, you are better placed to analyze that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After running these a few time for this PR, I'd say that this one won't make much of a difference. But I have identified several tests that instantiate its own model each that could be merged together. That, I think, will have a more significant impact. I'll open a PR about it once some of my other PRs are merged!

Copy link
Member

Choose a reason for hiding this comment

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

sounds good


∫c_dx = Field(Average(model.tracers.c, dims=(1)))
∫∫c_dxdy = Field(Average(model.tracers.c, dims=(1, 2)))
∫∫∫c_dxdydz = Field(Average(model.tracers.c, dims=(1, 2, 3)))

volume_avg_nc_filepath = "volume_averaged_field_test.nc"

simulation.output_writers[:averages] = NetCDFOutputWriter(model, (; ∫c_dx, ∫∫c_dxdy, ∫∫∫c_dxdydz),
array_type = Array{Float64},
verbose = true,
filename = volume_avg_nc_filepath,
schedule = TimeInterval(10Δt))
run!(simulation)

ds = NCDataset(volume_avg_nc_filepath)

for (n, t) in enumerate(ds["time"])
@test all(ds["∫c_dx"][:,:, n] .≈ 1)
@test all(ds["∫∫c_dxdy"][:, n] .≈ 1)
@test all(ds["∫∫∫c_dxdydz"][n] .≈ 1)
end

close(ds)

return nothing
end


function test_netcdf_time_averaging(arch)
topo = (Periodic, Periodic, Periodic)
domain = (x=(0, 1), y=(0, 1), z=(0, 1))
grid = RectilinearGrid(topology=topo, size=(4, 4, 4); domain...)
grid = RectilinearGrid(arch, topology=topo, size=(4, 4, 4); domain...)

λ1(x, y, z) = x + (1 - y)^2 + tanh(z)
λ2(x, y, z) = x + (1 - y)^2 + tanh(4z)
Expand Down Expand Up @@ -795,6 +838,7 @@ for arch in archs
test_thermal_bubble_netcdf_output_with_halos(arch)
test_netcdf_function_output(arch)
test_netcdf_output_alignment(arch)
test_netcdf_spatial_average(arch)
test_netcdf_time_averaging(arch)
test_netcdf_vertically_stretched_grid_output(arch)
test_netcdf_regular_lat_lon_grid_output(arch)
Expand Down