Uh oh!
There was an error while loading. Please reload this page.
Enable passing a CFTimedeltaCoder to decode_timedelta - #9966
Conversation
kmuehlbauer
commented
Jan 20, 2025
Yes, I was just playing around a bit. Please go ahead, @spencerkclark. I'll catch up tomorrow. |
kmuehlbauer
left a comment
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.
Uh oh!
There was an error while loading. Please reload this page.
| 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!
kmuehlbauer
commented
Jan 29, 2025
@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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
dcherian
commented
Jan 29, 2025
@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. |
kmuehlbauer
commented
Jan 29, 2025
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