-
Notifications
You must be signed in to change notification settings - Fork 164
Vectorized kernels in Parcels #2122
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
|
Quite excited to see the performance diff here! |
Check out Parcels-code/parcels-benchmarks#4 (comment)! |
|
Hi @fluidnumerics-joe, could you try to fix the breaking unit test in It seems related to the fact that ValueError: Points are neither Cartesian (shape N x 3) nor Spherical (shape N x 2).```python =================================== FAILURES =================================== _______________________ test_uxstommelgyre_pset_execute ________________________
tests/v4/test_particleset_execute.py:153: parcels/particleset.py:815: in execute points = array([[30.],
E ValueError: Points are neither Cartesian (shape N x 3) nor Spherical (shape N x 2). ../../anaconda3/envs/parcels-v4/lib/python3.12/site-packages/uxarray/grid/coordinates.py:753: ValueError |
|
Taking a look. I think these just need to be column stacked... "just" is a just a famous last word of course.. |
To handle array of lat,lon the x and y values need to be column stacked. After making this change, the internal API funciton in uxgrid (grid.neighbors._triangle_area) fails due to an assumed shape for the coordinate (being a scalar and not an array). The quick workaround here is to bring the `_barycentric_coordinates` method into parcels.uxgrid and leverage the numpy-ified parcels.uxgrid._triangle_area method we already have defined. In making this change, since order of operations is different for the barycentric coordinate calculation, this required updating the rounded and hashed values in the stommel gyre test. The actual values for the lat/lon still appear to be ok.
…nto vectorized-kernel
|
@erikvansebille - The stommel gyre test now passes - see the commit message in cb702e1 for details |
Do you want to also cherry-pick these changes into another PR too? Note that this PR may never be merged, if we decide that vectorization is not the way forward for performance; so this code may never get merged. Or is the change in the code not per se an improvement? |
|
I'd say it's an improvement in the sense we have more control within parcels to improve barycentric performance calculation without having to go through uxarray internal api. I'll make a note to get another PR in this afternoon with just that commit |
and also reverting some of the unit test assert changes (incl bac89b6); that are not needed anymore
And cleaning up the existing interpolators
By broadcasting to avoid repeated array access in bcoord computation Also, adding a note that searching can be much quicker when grid spacing is uniform
|
(+ updating the description with a summary of changes would be great) |
As suggested by @VeckoTheGecko in review of #2122 (#2122 (comment))
Including also a unit test with mixed outofbounds and valid locations
instead of the kernel-time; which is not well defined in vectorized form
Also implementing better error handling
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.
Some minor changes and comments for a future PR, nothing majorly blocking
Co-authored-by: Nick Hodgskin <[email protected]>
for more information, see https://pre-commit.ci
As suggested in #2122 (comment)
One way to improve performance in Parcels is to 'vectorize' the kernels: i.e. to not make kernels loop over particles, but to have them act on the entire particles. This PR is an implementation of that approach.
The kernel loop has been completely rewritten, and for Parcels users there will be a few important changes
if-statements anymore in their Kernels. Instead, they will need to applynp.where()or masked indexing. But note that even complex Kernels like RK45 with adaptive times-tapping are still possibleparticlebut a set ofparticlesand b)timedoes not have much meaning anymore when all particles could potentially have their own time. See also Reconsider Kernel signature for Parcels v4 #2147Nparticles, the memory requirements of temporary variables is now much more likely to grow quickly.The performance of this PR has been explored in Parcels-code/parcels-benchmarks#2 (comment)
mainfor v3 changes,v4-devfor v4 changes)