-
Notifications
You must be signed in to change notification settings - Fork 77
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
Bugfix use of datetime.datetime objects in call to get_last_an_time
#159
Conversation
… get last ascending node function Signed-off-by: Adam.Dybbroe <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 87.99% 88.12% +0.12%
==========================================
Files 14 14
Lines 2241 2265 +24
==========================================
+ Hits 1972 1996 +24
Misses 269 269
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pyorbital/orbital.py
Outdated
# Propagate backwards to ascending node | ||
dt = np.timedelta64(10, 'm') | ||
t_old = utc_time | ||
t_old = np.datetime64(utc_time) |
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.
FYI in modern python times a datetime object is "supposed to" have a timezone defined on it. If you use this method of conversion here and a python datetime object has a timezone then you'll get a warning:
In [13]: now = dt.datetime.now(dt.timezone.utc)
In [14]: np.datetime64(now)
<ipython-input-14-019101e2dae5>:1: UserWarning: no explicit representation of timezones available for np.datetime64
np.datetime64(now)
Out[14]: np.datetime64('2024-07-17T15:36:14.860486')
I think you could check if the datetime has a .tzinfo
and if not then convert it to UTC and then drop the timezone (not sure how to do that yet) before passing it to np.datetime64
to avoid the warning. Or I guess you could drop any timezone information no matter what because you have defined this interface as expecting time in UTC. 🤔
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.
Good point, thanks! I believe I am handling this now!?
Signed-off-by: Adam.Dybbroe <[email protected]>
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.
Looking good! One minor comment regarding readability
pyorbital/orbital.py
Outdated
if not hasattr(utc_time, 'tzinfo') or utc_time.tzinfo is None: | ||
return utc_time | ||
|
||
if utc_time.tzinfo != pytz.utc: | ||
raise AttributeError("UTC time expected! Parsing a timezone aware datetime object requires it to be UTC!") | ||
|
||
return utc_time.replace(tzinfo=None) |
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 think readability could be improved by separating the datetime path from the datetime64 path, for example
if isinstance(utc_time, dt.datetime):
if utc_time.tzinfo and utc_time.tzinfo != pytz.utc:
raise AttributeError
return utc_time.replace(tzinfo=None)
return utc_time
Signed-off-by: Adam.Dybbroe <[email protected]>
pyorbital/orbital.py
Outdated
""" | ||
if isinstance(utc_time, datetime): | ||
if utc_time.tzinfo and utc_time.tzinfo != pytz.utc: | ||
raise AttributeError("UTC time expected! Parsing a timezone aware datetime object requires it to be UTC!") |
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 think this should be a ValueError
. The AttributeError
I think is more to say you tried to access an attribute that didn't exist. ValueError
(in my opinion) is more like "you gave me a value that doesn't make sense".
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.
See my other previous comments for requested changes.
Agree with the value error, otherwise looks good to me! |
Co-authored-by: David Hoese <[email protected]>
Signed-off-by: Adam.Dybbroe <[email protected]>
Fix so a normal datetime.datetime object can be passed as well to the get last ascending node function.
Currently when running this code below:
you will get this kind of error:
THis PR seeks to solve these kind of errors, allowing to pass a simple
datetime.datetime
object to the function, and not onlynumpy.datetime64
objects as seems to have been the case until now.flake8 pyorbital