-
Notifications
You must be signed in to change notification settings - Fork 143
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
Moving the time interpolation down to within _Interpolator functions #1895
Moving the time interpolation down to within _Interpolator functions #1895
Conversation
Parcels previously would run an inetrpolation on time index ti and one on ti+1, and then at the end linearly interpolate the results. Here, we change that to do the time-interpolation closer to the InterpolationContext
As SGrid support will probably be removed in v4 anyways (see #1914)
will need to be fixed later, when we have consolidated interpoaltion API
Removing `spatial` in interpolation method names, as interpolation is not only about space anymore, but also 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.
I think it may be a good idea with these interpolation functions (where all the arguments are numeric) to force keyword only arguments. i.e.,
- def x(a, b, c):
+ def x(*, a, b, c):
>>> def x(*, a, b, c): pass
...
>>> x(1,2,3)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: x() takes 0 positional arguments but 3 were given
This forces users to use keyword arguments, preventing errors like accidentally switching yi, and xi in the function call which can be easily missed.
_get_data_temporalinterp(ti=ti, yi=yi + j, xi=xi + i)
It can become a bit repetitive when calling them, but I think it's worth it for the added safety? Thoughts @erikvansebille?
parcels/tools/_helpers.py
Outdated
def calculate_next_ti(ti, tau, tdim): | ||
"""Check if the time is beyond the last time in the field""" | ||
return tau > np.finfo(float).eps and ti < tdim - 1 |
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.
Could we rename to should_calculate_next_ti
to be clearer?
And refactor to
tau_is_significant = tau > np.finfo(float).eps
return tau_is_significant and ti < tdim - 1
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.
@erikvansebille I was just thinking wouldn't np.isequal(tau, 0.0)
have the save effect?
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.
Sure, but we would then need to do something like tau_is_significant = not np.isequal(tau, 0.0)
; would that be more readable, with the 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.
I think that sounds good
Co-authored-by: Nick Hodgskin (🦎 Vecko) <[email protected]>
…hub.com/OceanParcels/parcels into swap_space_and_time_interpolation_order
Parcels previously would run an interpolation on time index
ti
and one onti+1
, and then at the end linearly interpolate the results.In this PR, we change that to do the time-interpolation closer to the
InterpolationContext
. We need to discuss whether this is the right place now, or whether we want to push it even into the interpolators themselves. The latter would open possible support also non-linear time-interpolationHence, this is more a proof of concept than a full-fledged PR to be merged...
main
for v3 changes,v4-dev
for v4 changes)