-
Notifications
You must be signed in to change notification settings - Fork 139
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
Re-ordering of Kernel actions to vectorially sum particle displacements #1402
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As calling functions is not possible anymore with new implementation
This was referenced Jul 27, 2023
Closed
…ial_interpolation
As it was not advertised/used, and with the new lazy loading schemes is not needed. It does complicate the code significantly
As not needed anymore
Also small update to test_print in test_kernel_language
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…extloop From lon_towrite to lon_nextloop
…OceanParcels/parcels into vectorial_summing_zarr_approach
Made some changes to the text, no changes to code.
for more information, see https://pre-commit.ci
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(note: this PR supersedes #1388, which included attempts to write using parquet and sqlite. But the performance of these approaches was poor, so this PR will keep zarr writing.)
Until now, the way that
Kernel
-concatenation worked in Parcels meant that the execution of multipleKernels
was generally not commutable: for example first advection a particle and then letting it sink would lead to a different result than first sinking and then advecting. That is because particle positions were updated within each kernel, instead of the vectorial summing of displacements that would be more appropriate (and would ensure commutability).More problematically, while the particle positions would be updated within a kernel, the time was generally only updated at the end. This meant that
Field
-sampling could give unexpected results (e.g. when sampling was done at the new location but the old time).To solve these issues, in this PR I propose that individual Kernels compute displacements (
particle_dlon
,particle_dlat
andparticle_ddepth
) and that these displacements are only added to the particle locations when all kernels have been executed for a timestep dt.This means that the workflow for each particle
p
and each timet
becomes (here forparticle_dlon
, same forparticle_dlat
andparticle_ddepth
):particle_dlon = 0
particle.lon = particle.lon_nextloop
Kernels
, including samplingKernels
, and let theKernels
updateparticle_dlon
, when requiredt += dt
and particle locations toparticle.lon_nextloop = particle.lon + particle_dlon
p
is written this timestep, writeparticle.lon
(note, the concept for this loop was updated after #1422)
Note that this workflow means that particles will be written at
t=0
(and there is no need anymore for a separate initial values sampling step!), but not anymore att=runtime
(the last tilmestep), unless we also change that the particle loop is executed one time more at the end (tbd).Also, since particle locations in all Kernels will now be vectorially summed, we don't need the
SummedFields
class anymore, simplifying the codebase a bit.Issues/questions that need to be explored or implemented:
t=runtime
(see comment above)particles_backups
are stil needed now that particles are not moved in a KernelSince this is a major overhaul that can change results of a
pet.execute()
(so is not per se backward-compatible), I suggest to make this PR (when merged) part of v3.0.0