-
-
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
Enable passing a CFTimedeltaCoder
to decode_timedelta
#9966
base: main
Are you sure you want to change the base?
Changes from all commits
ff7650f
72d1d1c
714e17d
f33ed02
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,8 @@ | |
|
||
import numpy as np | ||
|
||
from xarray.coders import CFDatetimeCoder | ||
from xarray.coding import strings, times, variables | ||
from xarray.coders import CFDatetimeCoder, CFTimedeltaCoder | ||
from xarray.coding import strings, variables | ||
from xarray.coding.variables import SerializationWarning, pop_to | ||
from xarray.core import indexing | ||
from xarray.core.common import ( | ||
|
@@ -90,7 +90,7 @@ def encode_cf_variable( | |
|
||
for coder in [ | ||
CFDatetimeCoder(), | ||
times.CFTimedeltaCoder(), | ||
CFTimedeltaCoder(), | ||
variables.CFScaleOffsetCoder(), | ||
variables.CFMaskCoder(), | ||
variables.NativeEnumCoder(), | ||
|
@@ -114,7 +114,7 @@ def decode_cf_variable( | |
decode_endianness: bool = True, | ||
stack_char_dim: bool = True, | ||
use_cftime: bool | None = None, | ||
decode_timedelta: bool | None = None, | ||
decode_timedelta: bool | CFTimedeltaCoder | None = None, | ||
) -> Variable: | ||
""" | ||
Decodes a variable which may hold CF encoded information. | ||
|
@@ -158,6 +158,8 @@ def decode_cf_variable( | |
|
||
.. deprecated:: 2025.01.1 | ||
Please pass a :py:class:`coders.CFDatetimeCoder` instance initialized with ``use_cftime`` to the ``decode_times`` kwarg instead. | ||
decode_timedelta : None, bool, or CFTimedeltaCoder | ||
Decode cf timedeltas ("hours") to np.timedelta64. | ||
|
||
Returns | ||
------- | ||
|
@@ -171,7 +173,10 @@ def decode_cf_variable( | |
original_dtype = var.dtype | ||
|
||
if decode_timedelta is None: | ||
decode_timedelta = True if decode_times else False | ||
if isinstance(decode_times, CFDatetimeCoder): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we could add a FutureWarning, that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the one thing that is kind of awkward about this is that it will warn when anyone calls |
||
decode_timedelta = CFTimedeltaCoder(time_unit=decode_times.time_unit) | ||
else: | ||
decode_timedelta = True if decode_times else False | ||
|
||
if concat_characters: | ||
if stack_char_dim: | ||
|
@@ -193,7 +198,9 @@ def decode_cf_variable( | |
var = coder.decode(var, name=name) | ||
|
||
if decode_timedelta: | ||
var = times.CFTimedeltaCoder().decode(var, name=name) | ||
if not isinstance(decode_timedelta, CFTimedeltaCoder): | ||
decode_timedelta = CFTimedeltaCoder() | ||
var = decode_timedelta.decode(var, name=name) | ||
if decode_times: | ||
# remove checks after end of deprecation cycle | ||
if not isinstance(decode_times, CFDatetimeCoder): | ||
|
@@ -335,7 +342,10 @@ def decode_cf_variables( | |
decode_coords: bool | Literal["coordinates", "all"] = True, | ||
drop_variables: T_DropVariables = None, | ||
use_cftime: bool | Mapping[str, bool] | None = None, | ||
decode_timedelta: bool | Mapping[str, bool] | None = None, | ||
decode_timedelta: bool | ||
| CFTimedeltaCoder | ||
| Mapping[str, bool | CFTimedeltaCoder] | ||
| None = None, | ||
) -> tuple[T_Variables, T_Attrs, set[Hashable]]: | ||
""" | ||
Decode several CF encoded variables. | ||
|
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.
There is still some unresolved issue, where we should think about a way forward. At least we could document it, like this:
I'm not sure if it would be a good idea to change the default to
decode_times=False
in this cycle of changes (as discussed in #1621), but we could at least add a future warning whendecode_timedelta=None
. WDYT?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.
Yes, I think that is a good idea—thanks for reminding me about that issue. I'll work on an update when I get a chance.