Fix some FieldTimeSeries indexing bugs

[loisbaker]

This PR fixes some small indexing bugs for FieldTimeSeries described in #4100

• Changes the way that times are compared in

  Oceananigans.jl/src/OutputReaders/field_time_series.jl

  Line 252 in d1e9b46

  if all(time_range .≈ times) # good enough for most

  and

  Oceananigans.jl/src/OutputReaders/set_field_time_series.jl

  Line 10 in d1e9b46

  find_time_index(time::Number, file_times) = findfirst(t -> t ≈ time, file_times)

  so that ≈ is replaced with a comparison using a vector norm in the first case, and using an absolute tolerance based on a characteristic timescale in the second case. This avoids returning false due to the default relative error criterion when comparing values close to zero.
• Moves the time array that is indexed in

  Oceananigans.jl/src/OutputReaders/set_field_time_series.jl

  Line 25 in d1e9b46

  t = fts.times[n]

  to the CPU so errors aren't thrown when fts.times is a CuArray.

[navidcy]

Suggested change

	dt = mean(file_times[2:end] - file_times[1:end-1])
	timescale = mean(diff(file_times))

[glwagner]

Hmm, it's really the time increment between time points, so I think Δt is more appropriate. Just suggest using the Δ not dt for readability

[navidcy]

Or mean_Δt

[navidcy]

why don't we compute this within find_time_index? just a thought...
or something like:

function find_time_index(time::Number, file_times; time_scale = mean(diff(file_times)))
    ϵ = 100 * eps(eltype(file_times))
    return findfirst(t -> isapprox(t, time, atol=ϵ*time_scale), file_times)
end

[loisbaker]

Good point, I guess pre-computing it avoids recalculation at every iteration of the for loop it's used in? But if that's not an issue it would be simpler to compute within the function.

[glwagner]

Precomputing serves two purposes: 1) we don't recompute, which could become important for very long time series, and 2) as a point of design, this allows us to change the estimate of the characteristic time increment later. Right now we use the mean of all time increments, but this is not the only choice one might make.

[glwagner]

@loisbaker should we add tests at all? Ok if that's a todo for the future, but tests will ensure these changes are lasting

[loisbaker]

@loisbaker should we add tests at all? Ok if that's a todo for the future, but tests will ensure these changes are lasting

Sure, I can work on that - a test to check that find_time_index will pick the right index when the error between the two compared values is sufficiently small, and doesn't otherwise? Would you also make a test to check that the range conversion in field_time_series.jl only happens when the time values are sufficiently close to evenly spaced?

[glwagner]

@loisbaker should we add tests at all? Ok if that's a todo for the future, but tests will ensure these changes are lasting

Sure, I can work on that - a test to check that find_time_index will pick the right index when the error between the two compared values is sufficiently small, and doesn't otherwise? Would you also make a test to check that the range conversion in field_time_series.jl only happens when the time values are sufficiently close to evenly spaced?

Perhaps just a simple test that resembles the case you presented that illustrated the bug would work (the more fine-grained tests can be nice too, but I don't think necessary in this case and they are more work).

We can also do in a future PR, since I think this is otherwise read to merge -- your call!

[loisbaker]

@loisbaker should we add tests at all? Ok if that's a todo for the future, but tests will ensure these changes are lasting

Sure, I can work on that - a test to check that find_time_index will pick the right index when the error between the two compared values is sufficiently small, and doesn't otherwise? Would you also make a test to check that the range conversion in field_time_series.jl only happens when the time values are sufficiently close to evenly spaced?

Perhaps just a simple test that resembles the case you presented that illustrated the bug would work (the more fine-grained tests can be nice too, but I don't think necessary in this case and they are more work).

We can also do in a future PR, since I think this is otherwise read to merge -- your call!

Great, I'll put this on the to-do list for a future PR!