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

Docstring parameter descriptions are somewhat redundant and inconsistent

Open kandersolar opened this issue 3 years ago • 15 comments

Is your feature request related to a problem? Please describe. Conventions like pvlib's definition of surface_tilt and surface_azimuth are documented somewhat haphazardly in docstring parameter descriptions. This is objectionable in that it is redundant (same description appears in many places) and inconsistent (not all descriptions are the same for a particular parameter). For example many functions in pvlib.irradiance have parameter descriptions like this:

https://github.com/pvlib/pvlib-python/blob/df1c56e92d91701319adad4fe5cf98837edad0fb/pvlib/irradiance.py#L553-L555

while some others have this:

https://github.com/pvlib/pvlib-python/blob/df1c56e92d91701319adad4fe5cf98837edad0fb/pvlib/irradiance.py#L329

Describe the solution you'd like I'm proposing we convert the "Variables and Symbols" page into a sphinx glossary. Much like how :py:func: links to function definitions, sphinx glossaries allow you to link terms to their definitions using :term:`surface_tilt`. So a glossary like this:

.. glossary::

   surface_tilt
      Collector surface tilt angle, measured upwards from horizontal.
      Zero tilt means a horizontal surface (facing directly upwards).
      See also :term:`surface_azimuth`.

   ...

would let all surface_tilt docstring parameter descriptions reduce to something like this:

surface_tilt : numeric
    See :term:`surface_tilt` [degrees]

Describe alternatives you've considered The downside I see to extracting and centralizing this information is that the gallery definitions are only immediately accessible in the built sphinx docs, so help(pvlib.irradiance.klucher) and pvlib.irradiance.klucher? will be less useful (or, depending on your viewpoint, less cluttered). So there is an argument to be made that centralizing this information is not a net gain and we should continue including it in the docstrings themselves.

Additional context #1253 is vaguely relevant

kandersolar avatar Mar 09 '22 17:03 kandersolar

A compromise is short description (for REPL) + “for more detail description see :glossary:term“ or something like that. Then you have best of both worlds maybe?

mikofski avatar Mar 09 '22 18:03 mikofski

I have a number of times noticed the issue of inconsistent naming of the same variable in different places. So I'm definitely in favor of implementing sphinx glossary. However, I also share @mikofski's point that it would be nice to have a combination.

Perhaps for simple stuff like surface_tilt it is fine to only defer to the glossary as the name is descriptive and intuitive.

However, for less intuitive variables, such as acronyms, I would prefer a small description. For example, many users might not know what aoi stands for, so in this case, I would propose a combination like this:

aoi : numeric
    Angle of incidence, see :term:`aoi`

Then, the range, convention, and a more elaborate description can be provided in the glossary.

AdamRJensen avatar Mar 23 '22 21:03 AdamRJensen