pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.
https://pvlib-python.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.16k stars 979 forks source link

Deprecated `pvlib.atmosphere.first_solar_spectral_correction` not scheduled for removal #2130

Open echedey-ls opened 1 month ago

echedey-ls commented 1 month ago

Describe the bug pvlib.atmosphere.first_solar_spectral_correction was deprecated in v0.10.0 but it doesn't have an scheduled removal version. No issues or PRs open for it.

To Reproduce See https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.atmosphere.first_solar_spectral_correction.html And tests for https://github.com/pvlib/pvlib-python/blob/048b70ffa8a90c788491576e5057bcfc0667d8ea/pvlib/tests/test_atmosphere.py#L91C1-L94C65 do not show when it should fail.

Expected behavior pvlib.atmosphere.first_solar_spectral_correction to have disappeared in v0.11, or the repo to have the corresponding test failures set to some version.

Versions:

Additional context

1810

RDaxini commented 1 month ago

Just wondering something general about deprecations like this

In code such as:

first_solar_spectral_correction = deprecated(
    since='0.10.0',
    alternative='pvlib.spectrum.spectral_factor_firstsolar'
)(pvlib.spectrum.spectral_factor_firstsolar)

is it possible to hyperlink the alternative function? I did not quite understand the syntax of the alternative argument, with pvlib.spectrum.spectral_factor_firstsolar written twice --- once as a string and once in parentheses. I guess the string is the text in the warning on the docs page, right? So to hyperlink the function, would (pvlib.spectrum.spectral_factor_firstsolar) need to be (:py:func:`pvlib.spectrum.spectral_factor_firstsolar`) or does it not work like that?

echedey-ls commented 1 month ago

Yeah @RDaxini , that's also one of the hardest patterns of Python for me. Here, deprecated is a decorator. That is, a function that takes a function as an argument and returns another function with extra code/modifications. This is thanks to python defining a function just as another object, where properties like the code and the docstring belong to it.

(
    since='0.10.0',
    alternative='pvlib.spectrum.spectral_factor_firstsolar'
)

These are some other arguments the decorator takes (strings), that get used to create a warning admonition that gets put at the start of the docstring of the new function that will be returned.

The last parameter (pvlib.spectrum.spectral_factor_firstsolar) is the input function.

Additionally, the decorator adds a warning on every call of the function.

Lastly, the return function with extra code and modified docstring gets assigned to a variable. Since it's type is a function and it is listed on a table of contents, sphinx is able to use it's mangled docstring to create a page. And it can get used just as the decorated function, but from another namespace.

is it possible to hyperlink the alternative function?

This observation is very valuable IMO. https://github.com/pvlib/pvlib-python/blob/8e2bc9199f47ce98acfd7d91cf57f2589f41c252/pvlib/_deprecation.py#L137 would need to be modified. Two possibilities:

  1. Add the role to the general warning message. The warning-on-call message would contain the rST role markup.
  2. Make two different messages, or a switch parameter in the current message builder.

I don't have a preference.

Btw, this is the updated deprecation module of matplotlib with some new features we may consider: https://github.com/matplotlib/matplotlib/blob/main/lib/matplotlib/_api/deprecation.py

cwhanse commented 1 month ago

If the deprecation alternative was hyperlinked, where would it appear? It's outside the docstring so not on the readthedocs pages. The message is printed to the terminal window where a hyperlink isn't useful (AFAIK). Am I missing something?

echedey-ls commented 1 month ago

@cwhanse it appears in RTD too. https://pvlib-python.readthedocs.io/en/stable/reference/generated/pvlib.atmosphere.first_solar_spectral_correction.html

image

cwhanse commented 1 month ago

Aha, thank you.