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

Fixing non-second timedelta64 bug #1807

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion parcels/particlefile.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@
if len(once_ids) > 0:
Z[varout].vindex[ids_once] = pset.particledata.getvardata(var, indices_to_write_once)
else:
if max(obs) >= Z[varout].shape[1]:
if max(obs) >= Z[varout].shape[1]: # type: ignore[type-var]

Check warning on line 363 in parcels/particlefile.py

View check run for this annotation

Codecov / codecov/patch

parcels/particlefile.py#L363

Added line #L363 was not covered by tests
self._extend_zarr_dims(Z[varout], store, dtype=self.vars_to_write[var], axis=1)
Z[varout].vindex[ids, obs] = pset.particledata.getvardata(var, indices_to_write)

Expand Down
3 changes: 3 additions & 0 deletions parcels/tools/converters.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,13 @@
self.time_origin = time_origin
self.calendar: str | None = None
if isinstance(time_origin, np.datetime64):
self.time_origin = time_origin
self.calendar = "np_datetime64"
elif isinstance(time_origin, np.timedelta64):
self.time_origin = time_origin.astype("timedelta64[s]")

Check warning on line 63 in parcels/tools/converters.py

View check run for this annotation

Codecov / codecov/patch

parcels/tools/converters.py#L63

Added line #L63 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a concern that this may lose subsecond resolution?

>>> np.timedelta64(110, "ns").astype('timedelta64[s]')
np.timedelta64(0,'s')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using nanoseconds for timedelta64; better like this?

Copy link
Contributor

@VeckoTheGecko VeckoTheGecko Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. The issue stems with the actual comparison of datetime objects in .reltime(). I'm not entirely sure what datetime objects are coming into (or expected to come into) that function, so something more sophisticated may be needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK is fine; then let's discuss in person. Or feel free to propose something else, if you want

self.calendar = "np_timedelta64"
elif isinstance(time_origin, cftime.datetime):
self.time_origin = time_origin

Check warning on line 66 in parcels/tools/converters.py

View check run for this annotation

Codecov / codecov/patch

parcels/tools/converters.py#L66

Added line #L66 was not covered by tests
self.calendar = time_origin.calendar

def reltime(self, time: TimeConverter | np.datetime64 | np.timedelta64 | cftime.datetime) -> float | npt.NDArray:
Expand Down
12 changes: 12 additions & 0 deletions tests/tools/test_converters.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,18 @@ def test_TimeConverter_reltime_one_day():
assert tc.reltime(time) == ONE_DAY


def test_TimeConverter_timedelta64_float():
ONE_DAY = 24 * 60 * 60
tc = TimeConverter(np.timedelta64(0, "s"))
assert tc.reltime(1 * ONE_DAY) == 1 * ONE_DAY

tc = TimeConverter(np.timedelta64(0, "D"))
assert tc.reltime(1 * ONE_DAY) == 1 * ONE_DAY

tc = TimeConverter(np.timedelta64(0, "ns"))
assert tc.reltime(1 * ONE_DAY) == 1 * ONE_DAY


@pytest.mark.parametrize(
"x, y",
[
Expand Down
Loading