-
Notifications
You must be signed in to change notification settings - Fork 302
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
Fix start/end time properties of hrit_jma
reader
#2851
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2851 +/- ##
==========================================
- Coverage 95.96% 95.95% -0.02%
==========================================
Files 366 366
Lines 53593 53619 +26
==========================================
+ Hits 51433 51452 +19
- Misses 2160 2167 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 9807301260Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9807923958Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9971081072Details
💛 - Coveralls |
def dt64_to_datetime(self, dt64): | ||
"""Conversion of numpy.datetime64 to datetime objects.""" | ||
if isinstance(dt64, np.datetime64): | ||
return dt64.astype(dt.datetime) | ||
return dt64 |
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.
How are you able to straight remove this with no other changes in this file? Was nothing else using this?
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, it was only used once in the tests
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.
I had one question for something that was confusing me, otherwise looks good from what I can tell.
Are there any warnings generated when using numpy 2 and timezone aware python datetime objects?
now = dt.datetime.now(dt.timezone.utc)
np.datetime64(now)
^ produces a warning.
No I don't see any. In this case I'm going the other way round from datetime64 to datetime. Since datetime64 has no timezone support I wouldn't expect warnings. There are other timezone related warnings, though. For example:
|
Yeah in another PR we should fix those. I thought Panu had started but maybe that was just the "dt" import change. Anyway...should you maybe add a UTC timezone to these datetimes? Or maybe we'll remember to do that in the future utcnow PR? Probably not. 🤷♂️ |
In #2787 I just swapped |
Oh, and as a reference the |
Oh, I forgot about that, thanks! |
Maybe it makes more sense to change them all at once when we decided on the way forward? |
I also think the datetime aware stuff should happen in "one go" so that nothing breaks in any of the comparisons etc. |
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.
LGTM
Fix
start/end_time
properties ofhrit_jma
reader to returndatetime
objects (currently integers).Reason:
datetime64.astype(dt.datetime)
doesn't always return adatetime
object. (see numpy/numpy#20351).I've updated usages of
datetime64.astype(dt.datetime)
in other readers, too. Although it doesn't seem to be a problem there, because they use micro/millisecond precision.