Skip to content
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

Backward in time outputs not written at desired frequency #1722

Closed
willirath opened this issue Oct 3, 2024 · 4 comments · Fixed by #1723
Closed

Backward in time outputs not written at desired frequency #1722

willirath opened this issue Oct 3, 2024 · 4 comments · Fixed by #1723
Assignees
Labels

Comments

@willirath
Copy link
Collaborator

Parcels version

3.0.5

Description

Looks like a regression from 3.0.4 to 3.0.5.

Code sample

from datetime import timedelta
import xarray as xr
import numpy as np
import parcels

example_dataset_folder = parcels.download_example_dataset("MovingEddies_data")
fieldset = parcels.FieldSet.from_parcels(f"{example_dataset_folder}/moving_eddies")

pset = parcels.ParticleSet.from_list(
    fieldset=fieldset,
    pclass=parcels.JITParticle,
    lon=[3.3e5, 3.3e5],
    lat=[1e5, 2.8e5],
)

# run forward

output_file = pset.ParticleFile(
    name="EddyParticles.zarr",
    outputdt=timedelta(hours=1),
)
pset.execute(
    parcels.AdvectionRK4,
    runtime=timedelta(days=2),
    dt=timedelta(minutes=5),
    output_file=output_file,
)

# run backward

output_file = pset.ParticleFile(
    name="EddyParticles_Bwd.zarr",
    outputdt=timedelta(hours=1),
)
pset.execute(
    parcels.AdvectionRK4,
    runtime=timedelta(days=2),
    dt=-timedelta(minutes=5),
    output_file=output_file,
)

# check that timesteps match (up to a sign)

ds = xr.open_zarr("EddyParticles.zarr/")
ds_bwd = xr.open_zarr("EddyParticles_Bwd.zarr/")

np.testing.assert_almost_equal(
    ds.time.isel(trajectory=0).diff("obs").mean().compute().astype(float).data[()],
    - ds_bwd.time.isel(trajectory=0).diff("obs").mean().compute().astype(float).data[()],
)

With v3.0.5:

AssertionError: 
Arrays are not almost equal to 7 decimals
 ACTUAL: 3600000000000.0
 DESIRED: 86400000000000.0

With v3.0.4 it works.

@willirath
Copy link
Collaborator Author

Further hints: The output times appear to match the time steps of the fieldset.

@erikvansebille
Copy link
Member

Thanks for reporting @willirath, I'll take a look asap. This could be the same issue that @sruehs previously encountered (personal communication); this minimal example is super-useful!

@erikvansebille erikvansebille self-assigned this Oct 4, 2024
@erikvansebille erikvansebille moved this from Backlog to In progress in Parcels development Oct 4, 2024
@VeckoTheGecko
Copy link
Contributor

Git bisect points to ed11e43

@VeckoTheGecko
Copy link
Contributor

Ah, this was the change

- next_callback = starttime + callbackdt if dt > 0 else starttime - callbackdt
+ next_callback = starttime * np.sign(dt)

but we need

diff --git a/parcels/particleset.py b/parcels/particleset.py
index d7179cf3..72696eb6 100644
--- a/parcels/particleset.py
+++ b/parcels/particleset.py
@@ -1137,7 +1137,7 @@ class ParticleSet:
             next_output = starttime + dt
         else:
             next_output = np.inf * np.sign(dt)
-        next_callback = starttime * np.sign(dt)
+        next_callback = starttime + callbackdt * np.sign(dt)
 
         tol = 1e-12
         time = starttime

erikvansebille added a commit that referenced this issue Oct 4, 2024
This fixes #1722, by reverting a change in ed11e43

Thanks for the git-bisect hint, @VeckoTheGecko!
VeckoTheGecko added a commit that referenced this issue Oct 7, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Parcels development Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants