From 337532c0ecea2373160311288e2212ad6725891d Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Sat, 29 Jun 2024 12:01:35 -0400 Subject: [PATCH 1/7] MRC -- Selecting with string for cftime See discussion in #9138 This commit and pull request mostly serves as a staging group for a potential fix. Test with: ``` pytest xarray/tests/test_cftimeindex.py::test_cftime_noleap_with_str ``` --- xarray/tests/test_cftimeindex.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/xarray/tests/test_cftimeindex.py b/xarray/tests/test_cftimeindex.py index f6eb15fa373..c393b256982 100644 --- a/xarray/tests/test_cftimeindex.py +++ b/xarray/tests/test_cftimeindex.py @@ -988,6 +988,22 @@ def test_cftimeindex_calendar_property(calendar, expected): assert index.calendar == expected +@requires_cftime +def test_cftime_noleap_with_str(): + # https://github.com/pydata/xarray/issues/9138 + time = pd.date_range("2000-01-01", "2006-01-01", freq="D") + temperature = np.ones(len(time)) + da = xr.DataArray( + data=temperature, + dims=["time"], + coords=dict( + time=time, + ), + ) + da_noleap = da.convert_calendar("noleap") + da_noleap.sel(time=slice("2001", "2002")) + + @requires_cftime def test_empty_cftimeindex_calendar_property(): index = CFTimeIndex([]) From 0f1a5a2271e5522b5dd946d7f4f38f591211286e Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Sat, 29 Jun 2024 12:09:39 -0400 Subject: [PATCH 2/7] effectively remove fastpath --- xarray/core/indexes.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/xarray/core/indexes.py b/xarray/core/indexes.py index f25c0ecf936..c948c0b2765 100644 --- a/xarray/core/indexes.py +++ b/xarray/core/indexes.py @@ -580,19 +580,10 @@ def __init__( array: Any, dim: Hashable, coord_dtype: Any = None, - *, - fastpath: bool = False, ): - if fastpath: - index = array - else: - index = safe_cast_to_index(array) + index = safe_cast_to_index(array).copy() if index.name is None: - # make a shallow copy: cheap and because the index name may be updated - # here or in other constructors (cannot use pd.Index.rename as this - # constructor is also called from PandasMultiIndex) - index = index.copy() index.name = dim self.index = index @@ -607,7 +598,7 @@ def _replace(self, index, dim=None, coord_dtype=None): dim = self.dim if coord_dtype is None: coord_dtype = self.coord_dtype - return type(self)(index, dim, coord_dtype, fastpath=True) + return type(self)(index, dim, coord_dtype) @classmethod def from_variables( @@ -653,11 +644,6 @@ def from_variables( obj = cls(data, dim, coord_dtype=var.dtype) assert not isinstance(obj.index, pd.MultiIndex) - # Rename safely - # make a shallow copy: cheap and because the index name may be updated - # here or in other constructors (cannot use pd.Index.rename as this - # constructor is also called from PandasMultiIndex) - obj.index = obj.index.copy() obj.index.name = name return obj From 822c0b8e9b09c79ffffc3cb8d3878ba49b1288d4 Mon Sep 17 00:00:00 2001 From: Mark Harfouche Date: Sat, 29 Jun 2024 14:39:10 -0400 Subject: [PATCH 3/7] Add docstring --- doc/whats-new.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index c58f73cb1fa..361812a49b3 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -39,6 +39,9 @@ Bug fixes By `Pontus Lurcock `_. - Promote floating-point numeric datetimes before decoding (:issue:`9179`, :pull:`9182`). By `Justus Magin `_. +- Address regression introduced in :pull:`9002` that caused inability to access + cftime indexes in certain senerios. (:issue:`9138`, :pull:`9192`). + By `Mark Harfouche `_. Documentation From 8282c90b595867f896fe0b14ff48c4ab69e8b4e3 Mon Sep 17 00:00:00 2001 From: Spencer Clark Date: Sun, 7 Jul 2024 13:19:51 -0400 Subject: [PATCH 4/7] Revert "effectively remove fastpath" This reverts commit 0f1a5a2271e5522b5dd946d7f4f38f591211286e. --- xarray/core/indexes.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/xarray/core/indexes.py b/xarray/core/indexes.py index c948c0b2765..f25c0ecf936 100644 --- a/xarray/core/indexes.py +++ b/xarray/core/indexes.py @@ -580,10 +580,19 @@ def __init__( array: Any, dim: Hashable, coord_dtype: Any = None, + *, + fastpath: bool = False, ): - index = safe_cast_to_index(array).copy() + if fastpath: + index = array + else: + index = safe_cast_to_index(array) if index.name is None: + # make a shallow copy: cheap and because the index name may be updated + # here or in other constructors (cannot use pd.Index.rename as this + # constructor is also called from PandasMultiIndex) + index = index.copy() index.name = dim self.index = index @@ -598,7 +607,7 @@ def _replace(self, index, dim=None, coord_dtype=None): dim = self.dim if coord_dtype is None: coord_dtype = self.coord_dtype - return type(self)(index, dim, coord_dtype) + return type(self)(index, dim, coord_dtype, fastpath=True) @classmethod def from_variables( @@ -644,6 +653,11 @@ def from_variables( obj = cls(data, dim, coord_dtype=var.dtype) assert not isinstance(obj.index, pd.MultiIndex) + # Rename safely + # make a shallow copy: cheap and because the index name may be updated + # here or in other constructors (cannot use pd.Index.rename as this + # constructor is also called from PandasMultiIndex) + obj.index = obj.index.copy() obj.index.name = name return obj From 91e8b0d6179e095027a72f86ed51d1f7426abb44 Mon Sep 17 00:00:00 2001 From: Spencer Clark Date: Sun, 7 Jul 2024 14:01:38 -0400 Subject: [PATCH 5/7] Fix by reassigning coordinate --- xarray/coding/calendar_ops.py | 13 ++++++++++++- xarray/tests/test_calendar_ops.py | 25 ++++++++++++++++++++++++- xarray/tests/test_cftimeindex.py | 16 ---------------- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/xarray/coding/calendar_ops.py b/xarray/coding/calendar_ops.py index c4fe9e1f4ae..70d51f53be6 100644 --- a/xarray/coding/calendar_ops.py +++ b/xarray/coding/calendar_ops.py @@ -5,7 +5,11 @@ from xarray.coding.cftime_offsets import date_range_like, get_date_type from xarray.coding.cftimeindex import CFTimeIndex -from xarray.coding.times import _should_cftime_be_used, convert_times +from xarray.coding.times import ( + _STANDARD_CALENDARS, + _should_cftime_be_used, + convert_times, +) from xarray.core.common import _contains_datetime_like_objects, is_np_datetime_like try: @@ -222,6 +226,13 @@ def convert_calendar( # Remove NaN that where put on invalid dates in target calendar out = out.where(out[dim].notnull(), drop=True) + if use_cftime or calendar not in _STANDARD_CALENDARS: + # Reassign times to ensure time index of output is a CFTimeIndex + # (previously it was an Index due to the presence of NaN values). + # Note this is not needed in the case that the output time index is + # a DatetimeIndex, since DatetimeIndexes can handle NaN values. + out[dim] = CFTimeIndex(out[dim].data) + if missing is not None: time_target = date_range_like(time, calendar=calendar, use_cftime=use_cftime) out = out.reindex({dim: time_target}, fill_value=missing) diff --git a/xarray/tests/test_calendar_ops.py b/xarray/tests/test_calendar_ops.py index 7d229371808..13e9f7a1030 100644 --- a/xarray/tests/test_calendar_ops.py +++ b/xarray/tests/test_calendar_ops.py @@ -1,9 +1,10 @@ from __future__ import annotations import numpy as np +import pandas as pd import pytest -from xarray import DataArray, infer_freq +from xarray import CFTimeIndex, DataArray, infer_freq from xarray.coding.calendar_ops import convert_calendar, interp_calendar from xarray.coding.cftime_offsets import date_range from xarray.testing import assert_identical @@ -286,3 +287,25 @@ def test_interp_calendar_errors(): ValueError, match="Both 'source.x' and 'target' must contain datetime objects." ): interp_calendar(da1, da2, dim="x") + + +@requires_cftime +@pytest.mark.parametrize( + ("source_calendar", "target_calendar", "expected_index"), + [("standard", "noleap", CFTimeIndex), ("all_leap", "standard", pd.DatetimeIndex)], +) +def test_convert_calendar_produces_time_index( + source_calendar, target_calendar, expected_index +): + # https://github.com/pydata/xarray/issues/9138 + time = date_range("2000-01-01", "2002-01-01", freq="D", calendar=source_calendar) + temperature = np.ones(len(time)) + da = DataArray( + data=temperature, + dims=["time"], + coords=dict( + time=time, + ), + ) + converted = da.convert_calendar(target_calendar) + assert isinstance(converted.indexes["time"], expected_index) diff --git a/xarray/tests/test_cftimeindex.py b/xarray/tests/test_cftimeindex.py index c393b256982..f6eb15fa373 100644 --- a/xarray/tests/test_cftimeindex.py +++ b/xarray/tests/test_cftimeindex.py @@ -988,22 +988,6 @@ def test_cftimeindex_calendar_property(calendar, expected): assert index.calendar == expected -@requires_cftime -def test_cftime_noleap_with_str(): - # https://github.com/pydata/xarray/issues/9138 - time = pd.date_range("2000-01-01", "2006-01-01", freq="D") - temperature = np.ones(len(time)) - da = xr.DataArray( - data=temperature, - dims=["time"], - coords=dict( - time=time, - ), - ) - da_noleap = da.convert_calendar("noleap") - da_noleap.sel(time=slice("2001", "2002")) - - @requires_cftime def test_empty_cftimeindex_calendar_property(): index = CFTimeIndex([]) From cd13e027ccac8558c24b303796a20f375aed65f9 Mon Sep 17 00:00:00 2001 From: Spencer Clark Date: Sun, 7 Jul 2024 14:08:54 -0400 Subject: [PATCH 6/7] Update what's new entry --- doc/whats-new.rst | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/doc/whats-new.rst b/doc/whats-new.rst index 361812a49b3..19b9ac2631b 100644 --- a/doc/whats-new.rst +++ b/doc/whats-new.rst @@ -39,9 +39,11 @@ Bug fixes By `Pontus Lurcock `_. - Promote floating-point numeric datetimes before decoding (:issue:`9179`, :pull:`9182`). By `Justus Magin `_. -- Address regression introduced in :pull:`9002` that caused inability to access - cftime indexes in certain senerios. (:issue:`9138`, :pull:`9192`). - By `Mark Harfouche `_. +- Address regression introduced in :pull:`9002` that prevented objects returned + by py:meth:`DataArray.convert_calendar` to be indexed by a time index in + certain circumstances (:issue:`9138`, :pull:`9192`). By `Mark Harfouche + `_ and `Spencer Clark + `. Documentation From ccf9e2cba70d16f54ac7c12d352d702a96ad5f85 Mon Sep 17 00:00:00 2001 From: Spencer Clark Date: Sun, 7 Jul 2024 14:35:44 -0400 Subject: [PATCH 7/7] Simplify if condition --- xarray/coding/calendar_ops.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/xarray/coding/calendar_ops.py b/xarray/coding/calendar_ops.py index 70d51f53be6..6f492e78bf9 100644 --- a/xarray/coding/calendar_ops.py +++ b/xarray/coding/calendar_ops.py @@ -6,7 +6,6 @@ from xarray.coding.cftime_offsets import date_range_like, get_date_type from xarray.coding.cftimeindex import CFTimeIndex from xarray.coding.times import ( - _STANDARD_CALENDARS, _should_cftime_be_used, convert_times, ) @@ -226,7 +225,7 @@ def convert_calendar( # Remove NaN that where put on invalid dates in target calendar out = out.where(out[dim].notnull(), drop=True) - if use_cftime or calendar not in _STANDARD_CALENDARS: + if use_cftime: # Reassign times to ensure time index of output is a CFTimeIndex # (previously it was an Index due to the presence of NaN values). # Note this is not needed in the case that the output time index is