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

TimeConverter.reltime with time_origin = np.timedelta64 sensitive to precision #1738

Open
VeckoTheGecko opened this issue Oct 22, 2024 · 2 comments
Labels

Comments

@VeckoTheGecko
Copy link
Contributor

Parcels version

master

Description

The internals of TimeConverter with np.timedelta64 assumes that the precision is in seconds, when that might not actually be the case. Hence np.timedelta64(1, "D") - 1 gives 0 when in reality it should be treated in seconds all the way.

What would be a suitable precision for the object? I assume nanoseconds and then convert to float seconds for output?

Code sample

import cftime
import numpy as np

from parcels.tools.converters import TimeConverter, _get_cftime_datetimes

DAY = 24 * 60 * 60

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

    tc = TimeConverter(np.timedelta64(0, "D"))
    assert tc.reltime(1 * DAY) == 1 * DAY
________________________________________________ test_TimeConverter_timedelta64_float _________________________________________________

    def test_TimeConverter_timedelta64_float():
        tc = TimeConverter(np.timedelta64(0, "s"))
        assert tc.reltime(1 * DAY) == 1 * DAY
    
        tc = TimeConverter(np.timedelta64(0, "D"))
>       assert tc.reltime(1 * DAY) == 1 * DAY
E       assert np.float64(7464960000.0) == (1 * 86400)
E        +  where np.float64(7464960000.0) = reltime((1 * 86400))
E        +    where reltime = 0 days.reltime

tests/tools/test_converters.py:24: AssertionError
======================================================= short test summary info =======================================================
FAILED tests/tools/test_converters.py::test_TimeConverter_timedelta64_float - assert np.float64(7464960000.0) == (1 * 86400)
@erikvansebille
Copy link
Member

Yes, agree that this should be fixed

@VeckoTheGecko
Copy link
Contributor Author

To be clear, I don't think think this bug is really encountered in production due to how the TimeConverter class is used. Regardless, I think this would be good to implement for robustness

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

No branches or pull requests

2 participants