-
Notifications
You must be signed in to change notification settings - Fork 2
Description
Bug description
Callbacks that execute at a set frequency, such as NaNCheckCallback, typically use EveryCalendarDtSchedule to determine when to execute the callback.
When the integrator time is a float (e.g. seconds since time_to_date. The problematic line is this one (source):
time_ms = Dates.Millisecond(round(1_000 * time))
Apparently, the 1000 * time occasionally introduces errors that is much greater than machine precision (I have seen errors of 32 or 64). Then, in EveryCalendarDtSchedule we incorrectly return false because the current_time is 64 or 32 milliseconds before next_date.
What this affects
- Frequency based callbacks using EveryCalendarDtSchedule were quasiperiodically being skipped. This bug was discovered while I was trying to implement a photosynthesis model in ClimaLand (Add P-model to canopy photosynthesis models ClimaLand.jl#1185) that uses a callback to check for local noon.
Steps to reproduce
Add verbose output to time_to_date like so:
function time_to_date(time::AbstractFloat, start_date::Dates.DateTime)
time_ms_float = 1000 * time
time_ms_float_rounded = round(time_ms_float)
time_ms = Dates.Millisecond(time_ms_float_rounded)
println("time is a $(typeof(time)).\ntime remainder from 1 (pre-round) = $(time % 1), \nafter times 1000 = $(time_ms_float % 10), after round = $(time_ms_float_rounded % 10), \ntime_ms = $time_ms")
return start_date + time_ms
endThen, the following inputs in the REPL gives an offline example of this error:
julia> time = Float32(1.047012f6)
1.047012f6
julia> start_date = Dates.DateTime(2010, 1, 1, 7)
2010-01-01T07:00:00
julia> ClimaDiagnostics.time_to_date(time, start_date)
time is a Float32.
time remainder from 1 (pre-round) = 0.0,
after times 1000 = 8.0, after round = 8.0,
time_ms = 1047011968 milliseconds
2010-01-13T09:50:11.968
Verbose output from a simulation that produces this error: pmodel_integration_verbose_output_bug.txt
Potential solutions
- Switch over to using ITime instead of float
- It seems like explicitly casting 1000 to Float64(1000) works if time is a Float32
- It also seems like using Float64 for time fixes this issue, but this has not been extensively tested