Skip to content

[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

Merged
merged 21 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions pvlib/_deprecation.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,3 +316,66 @@ def wrapper(*args, **kwargs):
return finalize(wrapper, new_doc)

return deprecate


def renamed_kwarg_warning(since, old_param_name, new_param_name, removal=""):
"""
Decorator to mark a possible keyword argument as deprecated and replaced
with other name.

Raises a warning when the deprecated argument is used, and replaces the
call with the new argument name. Does not modify the function signature.

.. warning::
Ensure ``removal`` date with a ``fail_on_pvlib_version`` decorator in
the test suite.

.. note::
Not compatible with positional-only arguments.

.. note::
Documentation for the function may updated to reflect the new parameter
name; it is suggested to add a |.. versionchanged::| directive.

Parameters
----------
since : str
The release at which this API became deprecated.
old_param_name : str
The name of the deprecated parameter.
new_param_name : str
The name of the new parameter.
removal : str, optional
The expected removal version, in order to compose the Warning message.

Examples
--------
@renamed_kwarg('1.4.0', 'old_name', 'new_name')
def some_function(new_name=None):
pass
"""

def deprecate(func, old=old_param_name, new=new_param_name, since=since):
def wrapper(*args, **kwargs):
if old in kwargs:
if new in kwargs:
raise ValueError(
f"{func.__name__} received both '{old}' and '{new}', "
"which are mutually exclusive since they refer to the "
f"same parameter. Please remove deprecated '{old}'."
)
warnings.warn(
f"Parameter '{old}' has been renamed since {since}. "
f"and will be removed "
+ ("in {removal}." if removal else "soon.")
+ f"Please use '{new}' instead.",
_projectWarning,
stacklevel=2,
)
kwargs[new] = kwargs.pop(old)
return func(*args, **kwargs)

wrapper = functools.wraps(func)(wrapper)
return wrapper

return deprecate
22 changes: 14 additions & 8 deletions pvlib/solarposition.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1340,15 +1341,20 @@ def solar_zenith_analytical(latitude, hourangle, declination):
)


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``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the docs somehow indicated that times is still allowed, for now. Maybe we should we use deprecated instead of versionchanged during the deprecation period, and versionchanged afterwards?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@echedey-ls echedey-ls Oct 2, 2024

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -1376,14 +1382,14 @@ def hour_angle(times, longitude, equation_of_time):
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.

Expand Down
39 changes: 39 additions & 0 deletions pvlib/tests/test__deprecation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
"""
Test the _deprecation module.
"""

import pytest

from pvlib import _deprecation


@pytest.fixture
def renamed_kwarg_func():
"""Returns a function decorated by renamed_kwarg_warning."""
@_deprecation.renamed_kwarg_warning(
"0.1.0", "old_kwarg", "new_kwarg", "0.2.0"
)
def func(new_kwarg):
return new_kwarg

return func


def test_renamed_kwarg_warning(renamed_kwarg_func):
# assert no warning is raised when using the new kwarg
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."):
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()
Loading