Use `cftime` to avoid out of bounds `datetime` when decoding non-CF time coordinates #283

pochedls · 2022-07-28T01:58:42Z

Description

This PR is intended to address an issue in which datasets with non-cf-compliant times will not open if they go too far into the future.

Closes [Bug]: OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 2262-05-01 00:00:00 #282

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

If applicable:

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass with my changes (locally and CI/CD build)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

conda-env/dev.yml

xcdat/dataset.py

pochedls · 2022-07-28T02:09:46Z

@tomvothecoder – I added this PR to get things rolling; I think you'll have some insights to improve the code in places.

I got stuck resolving a mypi issue. If you are okay with the direction I am going, I can fix the unit tests.

tomvothecoder · 2022-07-28T18:04:37Z

Hi @pochedls, I'm taking a look right now. I'll also rebase this branch on the latest main.

add unit test add comments for test, cleanup extraneous code initial work on #282 Bugfix/278 cannot generate bounds (#281) * initial solution for #278 * add unit test * add comments for test, cleanup extraneous code

tomvothecoder

Hey @pochedls, I reviewed your changes and rebased the branch on the latest main.

There are some outdated review comments because I ended up doing some refactoring since I was debugging directly on your branch anyways.

The next thing to for you to do to review my changes and fix the tests.

xcdat/dataset.py

.pre-commit-config.yaml

xcdat/dataset.py

- Add `types-python-dateutil` to `mypy` dependencies - Update `ref_date` var to `ref_dt_obj` to avoid mypy error `error: Unsupported operand types for + ("str" and "relativedelta")` - Use dictionary unpacking for units variable - Use `datetime.strptime` instead of `pd.datetime()` which runs into the `pd.Timestamp` limitation - Add logger.warning when non-CF compliant time coords cannot be decoded

xcdat/dataset.py

codecov-commenter · 2022-07-28T22:27:19Z

Codecov Report

Merging #283 (c0d8703) into main (2169b07) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #283   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines         1146      1169   +23     
=========================================
+ Hits          1146      1169   +23

Impacted Files	Coverage Δ
xcdat/dataset.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2169b07...c0d8703. Read the comment docs.

- Fix tests

xcdat/dataset.py

tomvothecoder

My second pass, which includes test fixes and additional refactoring.

tests/test_dataset.py

xcdat/dataset.py

Co-authored-by: pochedls <pochedley@gmail.com>

pochedls · 2022-07-29T01:37:48Z

I modified decode_non_cf_time to return more specific cftime.Datetime objects (e.g, cftime.Datetime360Day and cftime.DatetimeGregorian), which seems to be what xarray does:

import xarray as xr

fn = '/p/css03/esgf_publish/CMIP6/CMIP/MOHC/UKESM1-1-LL/historical/r1i1p1f2/Amon/tas/gn/v20220512/tas_Amon_UKESM1-1-LL_historical_r1i1p1f2_gn_185001-194912.nc'
print(ds.time)

<xarray.DataArray 'time' (time: 1200)>
array([cftime.Datetime360Day(1850, 1, 16, 0, 0, 0, 0, has_year_zero=True),
cftime.Datetime360Day(1850, 2, 16, 0, 0, 0, 0, has_year_zero=True),
cftime.Datetime360Day(1850, 3, 16, 0, 0, 0, 0, has_year_zero=True), ...,
cftime.Datetime360Day(1949, 10, 16, 0, 0, 0, 0, has_year_zero=True),
cftime.Datetime360Day(1949, 11, 16, 0, 0, 0, 0, has_year_zero=True),
cftime.Datetime360Day(1949, 12, 16, 0, 0, 0, 0, has_year_zero=True)],
dtype=object)
Coordinates:

time (time) object 1850-01-16 00:00:00 ... 1949-12-16 00:00:00
height float64 ...
Attributes:
bounds: time_bnds
axis: T
long_name: time
standard_name: time

This also fixes some issues I had in subsetting. I had been using the file /p/user_pub/climate_work/pochedley1/cmip6_msu/ttt_Amon_UKESM1-0-LL_historical_r10i1p1f2_gr_185001-201412.nc. In applying ds.sel(time=('1850-01-01', '2014-12-31'), I got the following error:

TypeError: cannot compare cftime.datetime(1932, 7, 1, 0, 0, 0, 0, calendar='360_day', has_year_zero=True) and cftime.datetime(1850, 1, 1, 0, 0, 0, 0, calendar='standard', has_year_zero=False)

I could fix this by using cftime object in my .sel statement, but this now works with the string/slice arguments (because the time vector has the appropriate cftime types).

- Move try and except statements into `decode_non_cf_time()` - Extract function `_get_cftime_coords()` to encapsulate related logic from `decode_non_cf_time()`

tomvothecoder

Hi @pochedls, here's another pass of refactoring. I think we're in good shape now.

Along with xarray's get_date_type() that you cited, xarray also has convert_times() that we can use. After refactoring with these methods, the existing tests still passed.

Here's a diff of your last commit and this one:
3e9d09c (#283)

Side-note: I am going to try silence the logger warnings in the test suite.

tomvothecoder · 2022-08-01T18:51:38Z

xcdat/dataset.py

+    date_type = get_date_type(calendar)
+    coords = convert_times(offsets, date_type=date_type)


Instead of implementing our own logic, we can reuse xarray's get_date_type() (you cited this one) and convert_times() methods.

xarray.coding.cftime_offsets.get_date_type()
xarray.coding.times.convert_times()

I don't mind this. I initially used the xr function, but then I wasn't sure if we wanted/needed their _is_standard_calendar check. I guess this will never run, because we set use_cftime = True.

I think convert_times is hitting problems for years far into the future (convert_times is based on pandas):

import xcdat fn = '/p/user_pub/climate_work/pochedley1/cmip6_msu/spliced/ttt_Amon_MRI-ESM2-0_historical-ssp585_r1i1p1f1_gn_185001-230012.nc' ds = xcdat.open_dataset(fn)

File ~/code/xc

dat/xcdat/dataset.py:719, in _get_cftime_coords(ref_date, offsets, calendar, units)
716 # Convert the array of datetime objects into cftime objects based on
717 # the calendar type.
718 date_type = get_date_type(calendar)
--> 719 coords = convert_times(offsets, date_type=date_type)
721 return coords

File ~/bin/anaconda3/envs/xcdat_dev/lib/python3.9/site-packages/xarray/coding/times.py:451, in convert_times(times, date_type, raise_on_invalid)
448 return cftime_to_nptime(times, raise_on_invalid=raise_on_invalid)
449 if is_np_datetime_like(times.dtype):
450 # Convert datetime64 objects to Timestamps since those have year, month, day, etc. attributes
--> 451 times = pd.DatetimeIndex(times)
452 new = np.empty(times.shape, dtype="O")
453 for i, t in enumerate(times):

.
.
.

OutOfBoundsDatetime: Out of bounds nanosecond timestamp: 2262-05-01 00:00:00

Refer to my suggestion to update the dtype of the offsets np.ndarray from datetime64 to object.

I validated this fix using your example.

xcdat/dataset.py

tomvothecoder · 2022-08-02T20:46:54Z

xcdat/dataset.py

+    ref_datetime: datetime = parser.parse(ref_date, default=datetime(2000, 1, 1))
+    offsets = np.array(
+        [ref_datetime + rd.relativedelta(**{units: offset}) for offset in offsets],
+        dtype="datetime64",


Suggested change

dtype="datetime64",

dtype="object",

In the convert_times() method, there is a conditional that checks if the offsets np.ndarray dtype is_np_datetime_like (which then converts to pd.DatetimeIndex, and we don't want that due to the Timestamp limitations).

Source code: https://github.com/pydata/xarray/blob/3c98ec7d96cc4b46664850cc7a40af2bc184fea0/xarray/coding/times.py#L454-L460

Instead, by setting the dtype=object, it returns an np.ndarray of cftime objects.

Source code: https://github.com/pydata/xarray/blob/3c98ec7d96cc4b46664850cc7a40af2bc184fea0/xarray/coding/times.py#L461-L476

- Update logger warning

tomvothecoder

Unless you find any more issues, we should be good to merge.

pochedls commented Jul 28, 2022

View reviewed changes

conda-env/dev.yml Show resolved Hide resolved

pochedls commented Jul 28, 2022

View reviewed changes