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

Correct AveragedTimeInterval to use actuations #3721

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a52812b
correct windowed_time_average.jl
liuchihl Aug 21, 2024
5cf92ab
test the function test_netcdf_time_averaging in test_netcdf_output_wr…
liuchihl Sep 28, 2024
de3b9ec
test checkpointer
liuchihl Sep 28, 2024
6fcb230
change T from sch.interval to sch.window
liuchihl Oct 1, 2024
f0b3683
add progress messages and test cheeckpointer)
liuchihl Oct 3, 2024
db93a0a
print progress message every iteration
liuchihl Oct 10, 2024
071a466
add previous_interval_stop_time back & redefine actuation time
liuchihl Oct 18, 2024
55cac93
update advance_time_average! and schedule
liuchihl Oct 19, 2024
eb7d6da
manually force actuations to be the right value
liuchihl Oct 19, 2024
2f5ea4a
update test_netcdf_timeaverage.jl
liuchihl Oct 24, 2024
572a956
Merge branch 'main' into correct-averagedtimeinterval
liuchihl Oct 24, 2024
ca39ff4
Merge branch 'main' into correct-averagedtimeinterval
tomchor Nov 8, 2024
a0c725c
Remove `test_netcdf_timeaverage.jl`
Nov 14, 2024
1572ddd
Merge pull request #5 from hdrake/correct-averagedtimeinterval
liuchihl Nov 14, 2024
6c8fe1a
Merge branch 'main' into correct-averagedtimeinterval
Nov 14, 2024
cb6fa08
Merge pull request #6 from hdrake/correct-averagedtimeinterval
liuchihl Nov 14, 2024
6021a0e
Change timestep in time-averaging-interval test to flag rounding errors
Nov 14, 2024
090dd63
Merge pull request #7 from hdrake/correct-averagedtimeinterval
liuchihl Nov 14, 2024
78750aa
get rid of trailing white spaces
tomchor Nov 14, 2024
ce4bf18
Add warning about stride>1 and add test for JLD2 output writer
Nov 16, 2024
f1f5ed8
Merge branch 'main' into correct-averagedtimeinterval
Nov 16, 2024
efb5fe2
fix typo
liuchihl Nov 16, 2024
9eb4075
Merge pull request #8 from hdrake/correct-averagedtimeinterval
liuchihl Nov 16, 2024
85e90e8
undo fix typo
liuchihl Nov 16, 2024
409aa17
fix typo
liuchihl Nov 16, 2024
64b88bf
Merge branch 'main' into correct-averagedtimeinterval
tomchor Nov 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OutputWriters/windowed_time_average.jl
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ function (wta::WindowedTimeAverage)(model)
model.clock.iteration > 0 &&
@warn "Returning a WindowedTimeAverage before the collection period is complete."

stride(wta) > 1 && @warn "WindowedTimeAverage can be erroneous when stride > 1 and either the timestep is variable or there are floating point rounding errors in times, both of which result in a decoupling of the model clock times (used in the OutputWriters) and iteration numbers (used for stride)."
Copy link
Member

Choose a reason for hiding this comment

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

What does "or there are floating point rounding errors in times" mean?

I think with a warning like this, we need to have an issue that documents the problem and goes into a bit more detail (someone who encounters this might want to fix it)


return wta.result
end

Expand Down
141 changes: 140 additions & 1 deletion test/test_jld2_output_writer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,140 @@ function test_jld2_time_averaging_of_horizontal_averages(model)
return nothing
end

function test_jld2_time_averaging(arch)
# Test for both "nice" floating point number and one that is more susceptible
# to rounding errors
for Δt in [1/64, 0.01]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for Δt in [1/64, 0.01]
for Δt in (1/64, 0.01)

just asthetics, but a tuple is better than an array

# Results should be very close (rtol < 1e-5) for stride = 1.
# stride > 2 is currently not very robust and can give inconsistent
# results due to floating number errors that can result in very timesteps,
Copy link
Member

Choose a reason for hiding this comment

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

I think a word is missing. Very [what] timesteps?

# which essentially decouples the clock time from the iteration number.
# Can add stride > 1 cases to the following line to test them.
for (stride, rtol) in zip([1], [1.e-5])
@info " Testing time-averaging of NetCDF outputs [$(typeof(arch))] with stride of $(stride) and relative tolerance of $(rtol)"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@info " Testing time-averaging of NetCDF outputs [$(typeof(arch))] with stride of $(stride) and relative tolerance of $(rtol)"
@info " Testing time-averaging of JLD2 outputs [$(typeof(arch))] with stride of $(stride) and relative tolerance of $(rtol)"

topo = (Periodic, Periodic, Periodic)
domain = (x=(0, 1), y=(0, 1), z=(0, 1))
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)

Fc1(x, y, z, t, c1) = - λ1(x, y, z) * c1
Fc2(x, y, z, t, c2) = - λ2(x, y, z) * c2

c1_forcing = Forcing(Fc1, field_dependencies=:c1)
c2_forcing = Forcing(Fc2, field_dependencies=:c2)

model = NonhydrostaticModel(; grid,
timestepper = :RungeKutta3,
Copy link
Member

Choose a reason for hiding this comment

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

This is the default time stepper so you shouldn't need to specify it

tracers = (:c1, :c2),
forcing = (c1=c1_forcing, c2=c2_forcing))

set!(model, c1=1, c2=1)

Δt = 0.01 # Floating point number chosen conservatively to flag rounding errors
simulation = Simulation(model, Δt=Δt, stop_time=50Δt)

∫c1_dxdy = Field(Average(model.tracers.c1, dims=(1, 2)))
∫c2_dxdy = Field(Average(model.tracers.c2, dims=(1, 2)))

jld2_outputs = Dict("c1" => ∫c1_dxdy, "c2" => ∫c2_dxdy)
horizontal_average_jld2_filepath = "decay_averaged_field_test.jld2"

simulation.output_writers[:horizontal_average] =
JLD2OutputWriter(model, jld2_outputs,
schedule = TimeInterval(10Δt),
dir = ".",
with_halos = false,
filename = horizontal_average_jld2_filepath,
overwrite_existing = true)
Comment on lines +231 to +235
Copy link
Member

Choose a reason for hiding this comment

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

indent error here


multiple_time_average_jld2_filepath = "decay_windowed_time_average_test.jld2"
single_time_average_jld2_filepath = "single_decay_windowed_time_average_test.jld2"
window = 6Δt

single_jld2_output = Dict("c1" => ∫c1_dxdy)

simulation.output_writers[:single_output_time_average] =
JLD2OutputWriter(model, single_jld2_output,
schedule = AveragedTimeInterval(10Δt, window = window, stride = stride),
dir = ".",
with_halos = false,
filename = single_time_average_jld2_filepath,
overwrite_existing = true)
Comment on lines +245 to +249
Copy link
Member

Choose a reason for hiding this comment

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

indent error


simulation.output_writers[:multiple_output_time_average] =
JLD2OutputWriter(model, jld2_outputs,
schedule = AveragedTimeInterval(10Δt, window = window, stride = stride),
dir = ".",
with_halos = false,
filename = multiple_time_average_jld2_filepath,
overwrite_existing = true)
Comment on lines +253 to +257
Copy link
Member

Choose a reason for hiding this comment

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

indent error


run!(simulation)

##### For each λ, horizontal average should evaluate to
#####
##### c̄(z, t) = ∫₀¹ ∫₀¹ exp{- λ(x, y, z) * t} dx dy
##### = 1 / (Nx*Ny) * Σᵢ₌₁ᴺˣ Σⱼ₌₁ᴺʸ exp{- λ(i, j, k) * t}
#####
##### which we can compute analytically.

c1 = FieldTimeSeries(horizontal_average_jld2_filepath, "c1")
c2 = FieldTimeSeries(horizontal_average_jld2_filepath, "c2")

Nx, Ny, Nz = size(c1.grid)
xs, ys, zs = c1.grid.xᶜᵃᵃ[1:Nx], c1.grid.yᵃᶜᵃ[1:Ny], c1.grid.zᵃᵃᶜ[1:Nz]

c̄1(z, t) = 1 / (Nx * Ny) * sum(exp(-λ1(x, y, z) * t) for x in xs for y in ys)
c̄2(z, t) = 1 / (Nx * Ny) * sum(exp(-λ2(x, y, z) * t) for x in xs for y in ys)

for (n, t) in enumerate(c1.times)
@test all(isapprox.(c1[1, 1, :, n], c̄1.(zs, t), rtol=rtol))
@test all(isapprox.(c2[1, 1, :, n], c̄2.(zs, t), rtol=rtol))
end

# Compute time averages...
c̄1(ts) = 1/length(ts) * sum(c̄1.(zs, t) for t in ts)
c̄2(ts) = 1/length(ts) * sum(c̄2.(zs, t) for t in ts)

#####
##### Test strided windowed time average against analytic solution
##### for *single* NetCDF output
#####
c1_single = FieldTimeSeries(single_time_average_jld2_filepath, "c1")

window_size = Int(window/Δt)

@info " Testing time-averaging of a single NetCDF output [$(typeof(arch))]..."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@info " Testing time-averaging of a single NetCDF output [$(typeof(arch))]..."
@info " Testing time-averaging of a single JLD2 output [$(typeof(arch))]..."


for (n, t) in enumerate(c1_single.times[2:end])
averaging_times = [t - n*Δt for n in 0:stride:window_size-1 if t - n*Δt >= 0]
@test all(isapprox.(c1_single[1, 1, :, n+1], c̄1(averaging_times), rtol=rtol, atol=rtol))
end

#####
##### Test strided windowed time average against analytic solution
##### for *multiple* NetCDF outputs
#####

c2_multiple = FieldTimeSeries(multiple_time_average_jld2_filepath, "c2")

@info " Testing time-averaging of multiple NetCDF outputs [$(typeof(arch))]..."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@info " Testing time-averaging of multiple NetCDF outputs [$(typeof(arch))]..."
@info " Testing time-averaging of multiple JLD2 outputs [$(typeof(arch))]..."


for (n, t) in enumerate(c2_multiple.times[2:end])
averaging_times = [t - n*Δt for n in 0:stride:window_size-1 if t - n*Δt >= 0]
@test all(isapprox.(c2_multiple[1, 1, :, n+1], c̄2(averaging_times), rtol=rtol))
end

rm(horizontal_average_jld2_filepath)
rm(single_time_average_jld2_filepath)
rm(multiple_time_average_jld2_filepath)
end
end
return nothing
end

for arch in archs
# Some tests can reuse this same grid and model.
topo =(Periodic, Periodic, Bounded)
Expand Down Expand Up @@ -327,5 +461,10 @@ for arch in archs
#####

test_jld2_time_averaging_of_horizontal_averages(model)

#####
##### Time-averaging (same test as in NetCDFOutputWriter)
#####
test_jld2_time_averaging(arch)
end
end
end
Loading