-
Notifications
You must be signed in to change notification settings - Fork 195
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
Linear time interpolation in FieldTimeSeries
#3236
Conversation
Question: If time is |
Well, fractional indices work, just not I am not super familiar with what date-times supports in terms of operations, if it supports summation and division it is straightforward to extend indexing with to |
I think it can be done but it’s definitely not as straightforward as +, /. Happy to approve now and open issue to generalize the method for TimeDates later. |
Are we ok with this as a user interface? it's a little implicit. We could alternatively use a function-based API to make things a little more obvious, something like fts[1, 2, 3, 4] # get 4th time-index at_time(4, fts, 1, 2, 3) # linearly interpolate to t=4 mainly i'd be worried about issues like fts[1, 2, 3, 4] \ne fts[1, 2, 3, 4.0] which is rather easy to confuse? We also might be able to use syntax like fts[1, 2, 3, time=4] if that is performant. Or fts[1, 2, 3, Time(4)] |
Out of curiosity @simone-silvestri could you check fts2[1, 2, 3, 4] == fts2[1, 2, 3, 4.0] in your example with the |
the outcome depends on whether |
Another way would be to pass Oceananigans' This might be convenient because we usually pass the |
@navidcy I added a way to not interpolate so that in case 4 belongs to but this shows a bit the confusion because 4.0 here is a time and 4 is an index. |
We wouldn't have |
So I would propose a clock implementation for easy internal manipulation which calls the |
Why not a new type |
Basically I think the "convenience" of not having to define a new type is trivial. You can define |
I am thinking about the ease of use in bc, for example @inline time_interpolated_bc(i, j, grid, clock, fields, p) = p.time_array[i, j, 1, clock] vs @inline function time_interpolated_bc(i, j, grid, clock, fields, p)
time = Time(clock)
return p.time_array[i, j, 1, time]
end I find the first implementation a bit more straightforward, but we can also have both |
I propose we prioritize the user interface rather than the source code implementation, which basically has to be written once then left alone |
also worth checking out https://github.com/rafaqz/DimensionalData.jl the |
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
…to ss/rework-fts-1
Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
We will also have to figure out (perhaps in a future PR) how to insert boundary conditions to |
|
||
# Linear time interpolation | ||
function Base.getindex(fts::FieldTimeSeries, i::Int, j::Int, k::Int, time_index::Time) | ||
Ntimes = length(fts.times) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length
is slow for big vectors right? Should we allow this to be passed into Time
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we can implement something like
how_many_times(time::Time{<:Int}, times) = time.Ntimes
how_many_times(::Time, times) = length(times)
or mahybe you have a better name
if n₁ == n₂ # no interpolation | ||
return getindex(fts, i, j, k, n₁) | ||
end | ||
|
||
# fractional index | ||
@inbounds n = (n₂ - n₁) / (fts.times[n₂] - fts.times[n₁]) * (time - fts.times[n₁]) + n₁ | ||
return getindex(fts, i, j, k, n₂) * (n - n₁) + getindex(fts, i, j, k, n₁) * (n₂ - n) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if n₁ == n₂ # no interpolation | |
return getindex(fts, i, j, k, n₁) | |
end | |
# fractional index | |
@inbounds n = (n₂ - n₁) / (fts.times[n₂] - fts.times[n₁]) * (time - fts.times[n₁]) + n₁ | |
return getindex(fts, i, j, k, n₂) * (n - n₁) + getindex(fts, i, j, k, n₁) * (n₂ - n) | |
end | |
# fractional index | |
@inbounds n = (n₂ - n₁) / (fts.times[n₂] - fts.times[n₁]) * (time - fts.times[n₁]) + n₁ | |
fts_interpolated = getindex(fts, i, j, k, n₂) * (n - n₁) + getindex(fts, i, j, k, n₁) * (n₂ - n) | |
# Don't interpolate if n = 0. | |
return ifelse(n₁ == n₂, getindex(fts, i, j, k, n₁), fts_interpolated) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not to do the interpolation if it is not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I was actually looking at the other getindex, no in the kernel it's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It's not good practice to resolve other people's comments, unless they are not responding or something --- the reviewer gets priority to decide whether their comment has been resolved or not.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kernel is fast? I'm referring to the situation where this is embedded in another kernel. For example, this getindex
call can be embedded in the kernels that calculate model tendencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, a simple if condition should be okay here as there should not be problems of branch divergence (all threads should look at the same time).
Got it. Is that still the case in very complicated kernels, for example kernels that calculate 3D tendencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, linear interpolation in GPU kernels is fast. That is if threads access the same time index. There could be a situation in which different threads access different time indexes but it doesn't immediately come to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is whether that principle is still useful when the linear interpolation is surrounded by lots of other expensive operations, as can happen when this is used inside a tendency kernel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's another example: FieldTimeSeries
is used to store data for an advecting velocity field. This is the application you'd get if you're interested in, for example, advecting biogeochemical tracers with a velocity field derived from the ECCO state estimate. In that case the linear interpolation is inserted into an advection scheme.
* Linear time interpolation * fixed * little bug when n == n1 == n2 * time implementation * Update src/TimeSteppers/clock.jl Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com> * Update src/Units.jl Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com> * fix * Update src/Units.jl Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com> * add triple ticks * import * change --------- Co-authored-by: Gregory L. Wagner <wagner.greg@gmail.com>
Allow linear time interpolation between different indices of the field time series triggered by a Float64 (AbstractTime not supported at the moment) index.