pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

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

Open echedey-ls opened this issue 1 year ago • 5 comments

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:

  • pvlib.__version__: 0.11.0

Additional context #1810

echedey-ls avatar Jul 13 '24 10:07 echedey-ls

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?

RDaxini avatar Jul 16 '24 11:07 RDaxini

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

echedey-ls avatar Jul 17 '24 10:07 echedey-ls

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?

cwhanse avatar Jul 17 '24 14:07 cwhanse

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

image

echedey-ls avatar Jul 17 '24 14:07 echedey-ls

Aha, thank you.

cwhanse avatar Jul 17 '24 14:07 cwhanse