-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[MAINT]: Decorator to warn about changed parameter names & allow for deprecation period #2237
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
Changes from 12 commits
ec24a16
0ed8898
bade670
f5e46ef
1fdecb2
65e144b
fd0974f
1eb44a5
f58209f
58c0191
9921ad7
30e3c65
2a8b56d
455e9ad
e779f79
981f394
25bcf02
a8f8576
c185834
308e5f6
3cd34f3
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 |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
|
||
from pvlib import atmosphere, tools | ||
from pvlib.tools import datetime_to_djd, djd_to_datetime | ||
from pvlib._deprecation import renamed_kwarg_warning | ||
|
||
|
||
def get_solarposition(time, latitude, longitude, | ||
|
@@ -389,7 +390,8 @@ | |
return result | ||
|
||
|
||
def sun_rise_set_transit_spa(times, latitude, longitude, how='numpy', | ||
@renamed_kwarg_warning("0.11.2", "times", "date", "0.12") | ||
def sun_rise_set_transit_spa(date, latitude, longitude, how='numpy', | ||
delta_t=67.0, numthreads=4): | ||
""" | ||
Calculate the sunrise, sunset, and sun transit times using the | ||
|
@@ -404,8 +406,12 @@ | |
|
||
Parameters | ||
---------- | ||
times : pandas.DatetimeIndex | ||
date : pandas.DatetimeIndex | ||
Must be localized to the timezone for ``latitude`` and ``longitude``. | ||
|
||
.. deprecated:: 0.11.2 until 0.12.0 | ||
Renamed from ``times`` to ``date``. | ||
|
||
latitude : float | ||
Latitude in degrees, positive north of equator, negative to south | ||
longitude : float | ||
|
@@ -417,15 +423,15 @@ | |
delta_t : float or array, optional, default 67.0 | ||
Difference between terrestrial time and UT1. | ||
If delta_t is None, uses spa.calculate_deltat | ||
using times.year and times.month from pandas.DatetimeIndex. | ||
using date.year and date.month from pandas.DatetimeIndex. | ||
For most simulations the default delta_t is sufficient. | ||
numthreads : int, optional, default 4 | ||
Number of threads to use if how == 'numba'. | ||
|
||
Returns | ||
------- | ||
pandas.DataFrame | ||
index is the same as input `times` argument | ||
index is the same as input ``date`` argument | ||
columns are 'sunrise', 'sunset', and 'transit' | ||
|
||
References | ||
|
@@ -439,35 +445,36 @@ | |
lat = latitude | ||
lon = longitude | ||
|
||
# times must be localized | ||
if times.tz: | ||
tzinfo = times.tz | ||
# date must be localized | ||
if date.tz: | ||
tzinfo = date.tz | ||
else: | ||
raise ValueError('times must be localized') | ||
raise ValueError("'date' must be localized") | ||
|
||
# must convert to midnight UTC on day of interest | ||
times_utc = times.tz_convert('UTC') | ||
unixtime = _datetime_to_unixtime(times_utc.normalize()) | ||
date_utc = date.tz_convert('UTC') | ||
unixtime = _datetime_to_unixtime(date_utc.normalize()) | ||
|
||
spa = _spa_python_import(how) | ||
|
||
if delta_t is None: | ||
delta_t = spa.calculate_deltat(times_utc.year, times_utc.month) | ||
delta_t = spa.calculate_deltat(date_utc.year, date_utc.month) | ||
|
||
transit, sunrise, sunset = spa.transit_sunrise_sunset( | ||
unixtime, lat, lon, delta_t, numthreads) | ||
|
||
# arrays are in seconds since epoch format, need to conver to timestamps | ||
# arrays are in seconds since epoch format, need to convert to timestamps | ||
transit = pd.to_datetime(transit*1e9, unit='ns', utc=True).tz_convert( | ||
tzinfo).tolist() | ||
sunrise = pd.to_datetime(sunrise*1e9, unit='ns', utc=True).tz_convert( | ||
tzinfo).tolist() | ||
sunset = pd.to_datetime(sunset*1e9, unit='ns', utc=True).tz_convert( | ||
tzinfo).tolist() | ||
|
||
return pd.DataFrame(index=times, data={'sunrise': sunrise, | ||
'sunset': sunset, | ||
'transit': transit}) | ||
return pd.DataFrame( | ||
index=date, | ||
data={"sunrise": sunrise, "sunset": sunset, "transit": transit}, | ||
) | ||
|
||
|
||
def _ephem_convert_to_seconds_and_microseconds(date): | ||
|
@@ -1340,15 +1347,20 @@ | |
) | ||
|
||
|
||
def hour_angle(times, longitude, equation_of_time): | ||
@renamed_kwarg_warning("0.11.2", "times", "time", "0.12.0") | ||
def hour_angle(time, longitude, equation_of_time): | ||
""" | ||
Hour angle in local solar time. Zero at local solar noon. | ||
|
||
Parameters | ||
---------- | ||
times : :class:`pandas.DatetimeIndex` | ||
time : :class:`pandas.DatetimeIndex` | ||
Corresponding timestamps, must be localized to the timezone for the | ||
``longitude``. | ||
|
||
.. versionchanged:: 0.11.2 | ||
Renamed from ``times`` to ``time``. | ||
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. It would be nice if the docs somehow indicated that 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 don't see a point to state that it's still allowed. The less work the better so I vote for version changed only. Deprecating is already cumbersome and I'm hesitant to touch that require it 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. My line of thought was exactly the same of @AdamRJensen 's. However, I did hesitate about the message. It can be changed to something like: .. versionchanged:: 0.12.0
Renamed from ``times`` to ``time``. ``times`` was deprecated in ``0.11.2``. or .. versionchanged:: 0.11.2
Renamed from ``times`` to ``time``. ``times`` will be removed in ``0.12.0``. which explains both things but clutters the docs IMO. I don't mind changing it thou. 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. But it's true that in v0.12.0 a follow-up PR to clean up the test that I have pushed now in 65e144b is required Edit to complete the statement: a PR required that can also be used to clean up what is left and modify docs as needed. |
||
|
||
longitude : numeric | ||
Longitude in degrees | ||
equation_of_time : numeric | ||
|
@@ -1376,14 +1388,14 @@ | |
equation_of_time_pvcdrom | ||
""" | ||
|
||
# times must be localized | ||
if not times.tz: | ||
raise ValueError('times must be localized') | ||
# time must be localized | ||
if not time.tz: | ||
raise ValueError('time must be localized') | ||
|
||
# hours - timezone = (times - normalized_times) - (naive_times - times) | ||
tzs = np.array([ts.utcoffset().total_seconds() for ts in times]) / 3600 | ||
# hours - timezone = (time - normalized_time) - (naive_time - time) | ||
tzs = np.array([ts.utcoffset().total_seconds() for ts in time]) / 3600 | ||
|
||
hrs_minus_tzs = _times_to_hours_after_local_midnight(times) - tzs | ||
hrs_minus_tzs = _times_to_hours_after_local_midnight(time) - tzs | ||
|
||
return 15. * (hrs_minus_tzs - 12.) + longitude + equation_of_time / 4. | ||
|
||
|
echedey-ls marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
""" | ||
Test the _deprecation module. | ||
""" | ||
|
||
import pytest | ||
|
||
from pvlib import _deprecation | ||
|
||
import warnings | ||
|
||
|
||
@pytest.fixture | ||
def renamed_kwarg_func(): | ||
"""Returns a function decorated by renamed_kwarg_warning. | ||
This function is called 'func' and has a docstring equal to 'docstring'. | ||
""" | ||
|
||
@_deprecation.renamed_kwarg_warning( | ||
"0.1.0", "old_kwarg", "new_kwarg", "0.2.0" | ||
) | ||
def func(new_kwarg): | ||
"""docstring""" | ||
return new_kwarg | ||
|
||
return func | ||
|
||
|
||
def test_renamed_kwarg_warning(renamed_kwarg_func): | ||
# assert decorated function name and docstring are unchanged | ||
assert renamed_kwarg_func.__name__ == "func" | ||
assert renamed_kwarg_func.__doc__ == "docstring" | ||
|
||
# assert no warning is raised when using the new kwarg | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter("error") | ||
assert renamed_kwarg_func(new_kwarg=1) == 1 # as keyword argument | ||
assert renamed_kwarg_func(1) == 1 # as positional argument | ||
|
||
# assert a warning is raised when using the old kwarg | ||
with pytest.warns(Warning, match="Parameter 'old_kwarg' has been renamed"): | ||
assert renamed_kwarg_func(old_kwarg=1) == 1 | ||
|
||
# assert an error is raised when using both the old and new kwarg | ||
with pytest.raises(ValueError, match="they refer to the same parameter."): | ||
echedey-ls marked this conversation as resolved.
Show resolved
Hide resolved
|
||
renamed_kwarg_func(old_kwarg=1, new_kwarg=2) | ||
|
||
# assert when not providing any of them | ||
with pytest.raises( | ||
TypeError, match="missing 1 required positional argument" | ||
): | ||
renamed_kwarg_func() |
Uh oh!
There was an error while loading. Please reload this page.