Enable passing a CFTimedeltaCoder to decode_timedelta
#9966
Conversation
|
Yes, I was just playing around a bit. Please go ahead, @spencerkclark. I'll catch up tomorrow. |
There was a problem hiding this comment.
Thanks @spencerkclark! One suggestion for adding a FutureWarning and explicit mention of decode_times=False in the docs.
| @@ -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.
Here we could add a FutureWarning, that decode_timedelta=None will default to decode_timedelta=False. See #1621.
There was a problem hiding this comment.
I guess the one thing that is kind of awkward about this is that it will warn when anyone calls open_dataset, even if there are no timedelta-like variables in the dataset. Would it make sense to limit the warning to only when timedelta variables are decoded and decode_timedelta was None?
There was a problem hiding this comment.
Yes, It would be reasonable to reduce the noise for unaffected users. 👍
There was a problem hiding this comment.
I gave it a try in 3a96c8a—let me know what you think.
There was a problem hiding this comment.
Looks good! Thanks for the quick response!
|
@spencerkclark Any chance to get this over the finish line until Friday? Otherwise we should think about postponing the release date of the non-nanosecond refactoring into February. @dcherian beside this PR there is one more PR (#9999) which is connected to non-nanosecond and should go in before a release. WDYT, is it manageable to release until Friday? |
Co-authored-by: Kai Mühlbauer <kmuehlbauer@wradlib.org>
…erkclark/xarray into time_unit-decode_timedelta
for more information, see https://pre-commit.ci
…erkclark/xarray into time_unit-decode_timedelta
|
@kmuehlbauer I think you should handle the release :)
For this kind of coordination it's nice to open a release tracking issue so that anyone who isn't paying attention to this PR knows about it. |
|
Thanks @spencerkclark! Merging as needed for #10002- |
I had some time this weekend so I worked on enabling passing a
CFTimedeltaCodertodecode_timedeltainopen_datasetet al.@kmuehlbauer I didn't realize until this morning that you had started on this too in your fork (branch)—I was able to make a simplification to the
dtypeinference code I originally had based on looking at your initial work. I'm posting this here, since it seems a little further along than your branch, but feel free to supersede this PR with one of your own if you have un-pushed progress. I listed this PR in the same what's new entry as #9618, since it seemed natural, and gives us both credit.It looks like we were fairly aligned though in terms of inheriting the
time_unitfromdecode_timesif nothing was explicitly passed todecode_timedelta, which is good.whats-new.rst