Skip to content

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)

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
@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
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels development Mar 12, 2025
@github-project-automation github-project-automation bot moved this from Backlog to Done in Parcels v4 release Mar 12, 2025
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.

3 participants