Repository navigation
Fix changing dt in kernel - #3019
erikvansebille wants to merge 13 commits into
Conversation
| # TODO v4: need to implement ParticleFile writing of deleted particles | ||
| # if len(indices) > 0 and self.fieldset.particlefile is not None: | ||
| # self.fieldset.particlefile.write(pset, None, indices=indices) |
There was a problem hiding this comment.
Note that this TODO is not relevant now that we have https://docs.parcels-code.org/en/latest/user_guide/examples/tutorial_write_in_kernel.html#writing-on-particle-deletion, so can be ignored
As raised in tutorial_Delft3D
wyatt-fluidnumerics
left a comment
wyatt-fluidnumerics
left a comment
There was a problem hiding this comment.
Some thoughts below, though it is very possible I am miss-understanding the expected behavior of modifying dt in a kernel.
|
|
||
| sign_dt = np.sign(pset.dt[0]) | ||
| if np.any(np.sign(pset.dt) != sign_dt): | ||
| raise ValueError("All particle timesteps must have the same sign") |
There was a problem hiding this comment.
Would it be better for this to read, "same sign dt", to be more clear for users?
| # revert to original dt (unless in RK45 mode) | ||
| if not hasattr(self.fieldset, "RK45_tol"): | ||
| pset._data["dt"][:] = dt | ||
| # revert to the retained dt (unless in RK45 mode) |
There was a problem hiding this comment.
I am a bit confused here. Why revert to the original dt after the kernel is done with the timestep? Wouldn't this result in the same behavior that opened #2765 in the first place?
|
|
||
| pset._data["state"][:] = StatusCode.Success | ||
| preserve_particle_dt = not hasattr(self.fieldset, "RK45_tol") | ||
| dt_to_restore = pset.dt.copy() |
There was a problem hiding this comment.
Should dt_to_restore be updated somewhere in the while loop?
| runtime = sum(dt_in_kernel) | ||
| expected_num_kernel_loops = len(dt_in_kernel) | ||
| pset.execute(ModifyDt_FromArray, runtime=runtime, dt=1.0) | ||
|
|
There was a problem hiding this comment.
Should the test in #2765 be added too? As far as I can tell these tests don't actually verify that the particles.dt set on the previous timestep is retained on the following one. Only that changing dt during a step works for the current step.
(fyi I get the same behavior for this test on this branch as is documented in the original issue)
def test_dt_modify_in_kernel(fieldset):
TestParticle = Particle.add_variable(Variable("age", dtype=np.float32, initial=0))
pset = ParticleSet(fieldset, pclass=TestParticle, x=[0.5], y=[0])
def ModifyDt(particles, fieldset): # pragma: no cover
particles.age += particles.dt
particles.dt = 2
runtime = 10
expected_age = 1 + 2 * (runtime - 2) # 1 for the first step; 2 for the remaining steps (except last)
pset.execute(ModifyDt, runtime=runtime, dt=1.0)
np.testing.assert_allclose(pset.t[0], runtime)
np.testing.assert_allclose(pset.age[0], expected_age)
Description
This PR implements a fix to Issue #2765 where users couldn't change particles.dt inside the kernel. Instead of starting each kernel loop from the
dtargument ofpest.execute(), the particles now retain their own dtChecklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
Asked GPT6-Luna to provide initial suggestions with the prompt "Can you help me fix #2765". I then carefully cleaned up and simplified all suggested code changes myself