-
Notifications
You must be signed in to change notification settings - Fork 171
Bugfix/2568 output fenceposting #2575
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
base: main
Are you sure you want to change the base?
Changes from all commits
e9947ed
f75eefe
773c526
b456f0c
fc11251
51b1967
626675c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,8 +80,7 @@ def __init__( | |
|
|
||
| self._kernels: list[Callable] = kernels | ||
|
|
||
| if pset._requires_prepended_positionupdate_kernel: | ||
| self.prepend_positionupdate_kernel() | ||
| self._position_update = self._make_position_update() | ||
|
|
||
| @property #! Ported from v3. To be removed in v4? (/find another way to name kernels in output file) | ||
| def funcname(self): | ||
|
|
@@ -108,7 +107,7 @@ def remove_deleted(self, pset): | |
| if len(indices) > 0: | ||
| pset.remove_indices(indices) | ||
|
|
||
| def prepend_positionupdate_kernel(self): | ||
| def _make_position_update(self): | ||
| # Adding kernels that set and update the coordinate changes | ||
| def PositionUpdate(particles, fieldset): # pragma: no cover | ||
| particles.lon += particles.dlon | ||
|
|
@@ -124,7 +123,7 @@ def PositionUpdate(particles, fieldset): # pragma: no cover | |
| # Update dt in case it's increased in RK45 kernel | ||
| particles.dt = particles.next_dt | ||
|
|
||
| self._kernels = [PositionUpdate] + self._kernels | ||
| return PositionUpdate | ||
|
|
||
| def check_fieldsets_in_kernels(self, kernel): # TODO v4: this can go into another method? assert_is_compatible()? | ||
| """ | ||
|
|
@@ -221,6 +220,12 @@ def execute(self, pset, endtime, dt): | |
| f(pset[repeat_particles], self._fieldset) | ||
| repeat_particles = pset.state == StatusCode.Repeat | ||
|
|
||
| # apply position/time update only to particles still in a normal state | ||
| # (particles that signalled Stop*/Delete/errors should not have time/position advanced) | ||
|
Comment on lines
+223
to
+224
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So was this the fix to the bug in #2568? As simple as that?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fix was really to have explicit initial condition IO and file output after each call to positionupdate. The previous logic, which pre-pended positionupdate after the first call to kernel.execute() inadvertently wrote the initial condition twice, once for the initial time and once for the initial time + dt; this introduced the "off-by-dt" error in the particlefile output. |
||
| update_particles = evaluate_particles & np.isin(pset.state, [StatusCode.Evaluate, StatusCode.Success]) | ||
| if np.any(update_particles): | ||
| self._position_update(pset[update_particles], self._fieldset) | ||
|
|
||
| # revert to original dt (unless in RK45 mode) | ||
| if not hasattr(self.fieldset, "RK45_tol"): | ||
| pset._data["dt"][:] = dt | ||
|
|
@@ -244,9 +249,4 @@ def execute(self, pset, endtime, dt): | |
| else: | ||
| error_func(pset[inds].z, pset[inds].lat, pset[inds].lon) | ||
|
|
||
| # Only prepend PositionUpdate kernel at the end of the first execute call to avoid adding dt to time too early | ||
| if not pset._requires_prepended_positionupdate_kernel: | ||
| self.prepend_positionupdate_kernel() | ||
| pset._requires_prepended_positionupdate_kernel = True | ||
|
|
||
| return pset | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,7 +98,7 @@ def test_advection_zonal_periodic(): | |
| startlon = np.array([0.5, 0.4]) | ||
| pset = ParticleSet(fieldset, pclass=PeriodicParticle, lon=startlon, lat=[0.5, 0.5]) | ||
| pset.execute([AdvectionEE, periodicBC], runtime=np.timedelta64(40, "s"), dt=np.timedelta64(1, "s")) | ||
| np.testing.assert_allclose(pset.total_dlon, 4.1, atol=1e-5) | ||
| np.testing.assert_allclose(pset.total_dlon, 4.0, atol=1e-5) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice to see that you also fixed all these 'bugs'!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the extra update call that was there internally, there was a ton of these assertions that were right for the wrong reasons :) |
||
| np.testing.assert_allclose(pset.lon, startlon, atol=1e-5) | ||
| np.testing.assert_allclose(pset.lat, 0.5, atol=1e-5) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import numpy as np | ||
| import pytest | ||
|
|
||
| import parcels | ||
| from parcels._datasets.unstructured.generic import datasets as datasets_unstructured | ||
| from parcels.kernels import ( | ||
| AdvectionEE, | ||
| AdvectionRK2, | ||
| AdvectionRK4, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("integrator", [AdvectionEE, AdvectionRK2, AdvectionRK4]) | ||
| def test_ux_constant_flow_face_centered_2D(integrator): | ||
| ds = datasets_unstructured["ux_constant_flow_face_centered_2D"] | ||
| T = np.timedelta64(3600, "s") | ||
| dt = np.timedelta64(300, "s") | ||
| dt_s = 300.0 | ||
|
|
||
| fieldset = parcels.FieldSet.from_ugrid_conventions(ds, mesh="flat") | ||
| pset = parcels.ParticleSet(fieldset, lon=[5.0], lat=[5.0]) | ||
| pfile = parcels.ParticleFile(store="test.zarr", outputdt=dt) | ||
| pset.execute(integrator, runtime=T, dt=dt, output_file=pfile, verbose_progress=False) | ||
| expected_lon = 8.6 | ||
| np.testing.assert_allclose(pset.lon, expected_lon, atol=1e-5) | ||
|
Comment on lines
+22
to
+25
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the last locations in the output are not tested here. Also test that these are as expected? |
||
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.
Why not directly call
self._make_position_update()? Why (effectively) rename the function here?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.
That would be much simpler than what I concocted.