-
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
Fix various solar position calculations when DatetimeIndex.unit
is not ns
#1948
Conversation
Ready for review. The codecov decrease can be ignored, I think. It is because coverage is based on the python 3.7 job, which is too old to support the pandas version needed to exercise the tested behavior. That means the new tests are not being run on the 3.7 job, and so their lines are counting against coverage. However, you can see that the jobs with later python versions are indeed running them (the skips are for Additionally: we probably don't want to include it as a formal test due to the polars dependency, but I'll note that this PR also passes the reproducer code in #1932 (comment). |
2 * times.view(np.int64) - times.normalize().view(np.int64) - | ||
naive_times.view(np.int64)) | ||
if times.tz is None: | ||
times = times.tz_localize('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.
Should we warn
about this localization? Maybe just a note. The function promises hour angle in the time zone for the longitude.
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.
In case it's not obvious, this localization retains the existing behavior of assuming naive inputs to be in UTC; the rewrite for the unit
issue just happens to make it more explicit. See for v0.9.5:
In [1]: import pvlib # v0.9.5
...: import pandas as pd
...: for tz in [None, 'utc', 'Etc/GMT+5']:
...: times = pd.date_range('2019-06-01 12:00', freq='h', periods=2, tz=tz)
...: eot = pvlib.solarposition.equation_of_time_spencer71(times.dayofyear)
...: ha = pvlib.solarposition.hour_angle(times, -80, eot)
...: print(f'hour angles, {tz=}:', ha)
...:
hour angles, tz=None: [-79.36192279 -64.36192279]
hour angles, tz='utc': [-79.36192279 -64.36192279]
hour angles, tz='Etc/GMT+5': [-4.36192279 10.63807721]
The docstring says that times
must be localized (Corresponding timestamps, must be localized to the timezone for the longitude.
), so I guess naive times are technically not supported. Other solar position functions assume UTC for naive inputs, and don't warn, so I think I'm inclined to keep that behavior here as well.
docs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.