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

Moving the time interpolation down to within _Interpolator functions #1895

Merged
merged 35 commits into from
Mar 12, 2025

Conversation

erikvansebille
Copy link
Member

Parcels previously would run an interpolation on time index ti and one on ti+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-interpolation

Hence, this is more a proof of concept than a full-fledged PR to be merged...

  • Chose the correct base branch (main for v3 changes, v4-dev for v4 changes)

@erikvansebille erikvansebille marked this pull request as ready for review March 7, 2025 14:45
Removing `spatial` in interpolation method names, as interpolation is not only about space anymore, but also time
Copy link
Contributor

@VeckoTheGecko VeckoTheGecko left a 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?

Comment on lines 157 to 159
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
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko Mar 12, 2025

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

@erikvansebille erikvansebille merged commit c8d8e6d into v4-dev Mar 12, 2025
16 checks passed
@erikvansebille erikvansebille deleted the swap_space_and_time_interpolation_order branch March 12, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants