-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update time coding tests to assert exact equality #9961
base: main
Are you sure you want to change the base?
Conversation
if np.timedelta64(1, time_unit) > np.timedelta64(1, minimum_resolution): | ||
expected_unit = minimum_resolution | ||
else: | ||
expected_unit = time_unit | ||
expected = cftime_to_nptime(expected, time_unit=expected_unit) |
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.
I needed to add this logic specifically for this test:
(12300 + np.arange(5), "hour since 1680-01-01 00:00:00.500000", "us"),
This is because the reference date required the dates be decoded to microsecond resolution (really only millisecond resolution would be required, but we follow pandas's lead here). Otherwise for time_unit="s"
we would truncate precision when converting the cftime objects to np.datetime64
values, which prevented asserting exact equality.
@@ -220,7 +220,7 @@ def test_decode_standard_calendar_inside_timestamp_range( | |||
) -> None: | |||
import cftime | |||
|
|||
units = "days since 0001-01-01" | |||
units = "hours since 0001-01-01" |
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.
Using encoding units of "days" unnecessarily led the times to be encoded with floats, which prevented asserting exact equality in this test. Testing floating point time decoding was not the point of this test, so I changed to encoding the times with units of "hours" instead.
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.
Thanks @spencerkclark, this is looking good.
@spencerkclark Adding the plan-to-merge label. Is there anything you want to add here? Otherwise this is good to merge AFAICT. |
Per #9618 (comment), this PR updates relevant time coding tests to assert exact equality rather than equality with a tolerance, since xarray's minimum supported version of cftime has been greater than 1.2.1 for a while now.
cc: @kmuehlbauer
whats-new.rst