-
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
Add tests for time conversions in tools package #2341
base: main
Are you sure you want to change the base?
Changes from 17 commits
9347f12
0638773
c1df9a7
77c0f81
6704d06
dbb1805
6750709
1144106
14715ed
545c196
271fd97
01263c2
60a5d94
9ab2ecf
ddef8d1
4f17f49
a3c3e03
5f59417
c84801f
195efbc
1a5efed
e5af9ae
67e9844
e35eb42
a1a0261
8373ac4
eee6f51
9662c1f
32284ba
01e4cfc
c09a328
4ef4b69
7490792
a5f7646
1382e30
059e35f
f9f07d7
75db2aa
1164c96
5f6ad14
f691bb6
7cfb170
ef5c60f
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 | ||||
---|---|---|---|---|---|---|
|
@@ -12,12 +12,21 @@ Enhancements | |||||
~~~~~~~~~~~~ | ||||||
|
||||||
|
||||||
Bug Fixes | ||||||
~~~~~~~~~ | ||||||
* Ensure proper tz and pytz types in pvlib.location.Location. | ||||||
(:issue:`2340`, :pull:`2341`) | ||||||
|
||||||
|
||||||
Documentation | ||||||
~~~~~~~~~~~~~ | ||||||
|
||||||
|
||||||
Testing | ||||||
~~~~~~~ | ||||||
* Add tests for timezone types in pvlib.location.Location. | ||||||
(:issue:`2340`, :pull:`2341`) | ||||||
* Add tests for time conversions in pvlib.tools. (:issue:`2340`, :pull:`2341`) | ||||||
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.
Suggested change
|
||||||
|
||||||
|
||||||
Requirements | ||||||
|
@@ -26,5 +35,4 @@ Requirements | |||||
|
||||||
Contributors | ||||||
~~~~~~~~~~~~ | ||||||
|
||||||
|
||||||
* Mark Campanellli (:ghuser:`markcampanelli`) | ||||||
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.
Suggested change
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,11 +72,12 @@ def __init__(self, latitude, longitude, tz='UTC', altitude=None, | |
self.tz = 'UTC' | ||
self.pytz = pytz.UTC | ||
elif isinstance(tz, datetime.tzinfo): | ||
# This includes pytz timezones. | ||
self.tz = tz.zone | ||
self.pytz = tz | ||
self.pytz = pytz.timezone(tz.zone) | ||
elif isinstance(tz, (int, float)): | ||
self.tz = tz | ||
cwhanse marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.pytz = pytz.FixedOffset(tz*60) | ||
self.tz = f"Etc/GMT{int(-tz):+d}" | ||
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'm inclined to ask to revert this change. The docstring says 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. There is a point in the Location docstring that says this:
While I appreciate my changes may break some things, I think this existing docstring suggests a much more sane approach. (Indeed, in the sequel using 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'm not saying revert from storing a string, but revert the I'm open to doing away with accepting 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. @cwhanse Sorry I misunderstood your meaning. I was aware that some locations are on half-hour offsets and such, but what is unclear to me is if/how that is represented in I must be missing something here. Does something non-standard have to be done to support fractional offsets? 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.
Hi again @cwhanse. It looks like the standard "non-integral-hour" offsets are only supported in the IANA standard through place names such as "Asia/Kathmandu" and "Asia/Yangon", but not through Given that, I think the most trouble-free thing to do is to remove the Keeping both fields 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'm in favor of this change. @pvlib/pvlib-maintainer @pvlib/pvlib-triage your opinions? 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. No opinion. 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. TMY3 files represent timezone in numeric form, so this change may complicate some workflows. Outside of that, I never use this attribute (and don't know why I would), so I have no opinion either. Remember that we usually have a deprecation period before outright removal. 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 did some more investigating on this yesterday, and I think I see an implementation that would better support the existing 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. Ok. I have added support back for int and non-fractional float being passed to Note that the Ready for another review. |
||
self.pytz = pytz.timezone(self.tz) | ||
else: | ||
raise TypeError('Invalid tz specification') | ||
|
||
|
@@ -89,8 +90,10 @@ def __init__(self, latitude, longitude, tz='UTC', altitude=None, | |
|
||
def __repr__(self): | ||
attrs = ['name', 'latitude', 'longitude', 'altitude', 'tz'] | ||
# Use None as getattr default in case __repr__ is called during | ||
# initialization before all attributes have been assigned. | ||
return ('Location: \n ' + '\n '.join( | ||
f'{attr}: {getattr(self, attr)}' for attr in attrs)) | ||
f'{attr}: {getattr(self, attr, None)}' for attr in attrs)) | ||
|
||
@classmethod | ||
def from_tmy(cls, tmy_metadata, tmy_data=None, **kwargs): | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -119,21 +119,21 @@ def atand(number): | |||||
|
||||||
def localize_to_utc(time, location): | ||||||
""" | ||||||
Converts or localizes a time series to UTC. | ||||||
Converts time to UTC, localizing if necessary using location. | ||||||
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.
Suggested change
|
||||||
|
||||||
Parameters | ||||||
---------- | ||||||
time : datetime.datetime, pandas.DatetimeIndex, | ||||||
or pandas.Series/DataFrame with a DatetimeIndex. | ||||||
location : pvlib.Location object | ||||||
location : pvlib.Location object (unused if time is localized) | ||||||
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.
Suggested change
|
||||||
|
||||||
Returns | ||||||
------- | ||||||
pandas object localized to UTC. | ||||||
datetime.datetime or pandas object localized to UTC. | ||||||
""" | ||||||
if isinstance(time, dt.datetime): | ||||||
if time.tzinfo is None: | ||||||
time = pytz.timezone(location.tz).localize(time) | ||||||
time = location.pytz.localize(time) | ||||||
time_utc = time.astimezone(pytz.utc) | ||||||
else: | ||||||
try: | ||||||
|
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.