-
Notifications
You must be signed in to change notification settings - Fork 164
Implementing Field interpolation API #2332
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
Conversation
…y as suggested in #2287
for more information, see https://pre-commit.ci
src/parcels/_core/field.py
Outdated
| particle_positions = {"time": time, "z": z, "lat": y, "lon": x} | ||
| grid_positions = {} | ||
| grid_positions.update(_search_time_index(self.U, time)) | ||
| grid_positions.update(self.grid.search(z, y, x, ei=_ei)) | ||
| _update_particles_ei(particles, grid_positions, self) | ||
| _update_particle_states_position(particles, grid_positions) |
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 updated the API to reflect @erikvansebille’s and @VeckoTheGecko’s proposed structure (#2287) with two dictionaries, particle_positions and grid_positions. It seems like everything works as before, but I wasn’t 100% sure about initializing the dictionaries here within field.eval(); is there a nicer/better way to do this?
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 creating a dictionary like this is fine.
What I rather wonder is whether we should move this duplicate code (for Field.eval() and for VectorField.eval() into one helper function?
|
Yes, a review would be very useful, @VeckoTheGecko. And ideally also one from @fluidnumerics-joe to check if this Interpolation-API would also work for unstructured grids |
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.
Just a couple minor changes in the interpolators.py
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.
Looks good
This PR implements the proposal for a simpler Field interpolation API (#2287).
Note that it's blocked by #2294