Skip to content
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

Docstring parameter descriptions are somewhat redundant and inconsistent #1421

Open
kandersolar opened this issue Mar 9, 2022 · 15 comments
Open

Comments

@kandersolar
Copy link
Member

kandersolar commented Mar 9, 2022

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:

Surface tilt angles in decimal degrees. Tilt must be >=0 and
<=180. The tilt angle is defined as degrees from horizontal
(e.g. surface facing up = 0, surface facing horizon = 90).

while some others have this:

Panel tilt from horizontal. [degree]

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

@mikofski
Copy link
Member

mikofski commented Mar 9, 2022

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?

@AdamRJensen
Copy link
Member

AdamRJensen commented Mar 23, 2022

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.

@RDaxini
Copy link
Contributor

RDaxini commented Sep 11, 2024

Is anyone currently working on this? If not, I am very interested in taking on this work. I have a few projects running currently though so I might be a bit slow, but I guess since this is unresolved since 2022 we'd be okay with a few months more? 😅

@cwhanse
Copy link
Member

cwhanse commented Sep 11, 2024

Can we convert the "Variables and Symbols" page to a glossary as a first step? I am hesitant to remove the descriptions from docstrings. For those browsing the code, using the command line to look at documentation, or reviewing changes, having AOI spelled out along with "see :term:aoi" would be a little frustrating.

@RDaxini
Copy link
Contributor

RDaxini commented Sep 12, 2024

Can we convert the "Variables and Symbols" page to a glossary as a first step?

Sounds reasonable to me 👍🏾

@RDaxini
Copy link
Contributor

RDaxini commented Sep 29, 2024

Can we convert the "Variables and Symbols" page to a glossary as a first step?

#2234


There are some points to discuss about what we want from this glossary page and how we want to link terms in the documentation.

The current variables and symbols page defines function parameters as they appear in the function, e.g. lowercase dni. In the function docstring, we also have 'normal text', e.g. DNI or Direct Normal Irradiance.

Questions:
Do we want to link both instances to the glossary terms?
Do we want the glossary terms to be as written in the function or another form, e.g. expanded abbreviations?

If we want to link both instances, and use the parameter values directly as glossary terms, the docstring would need to look something like this:

:term:`Direct Normal Irradiance<dni>`, extraterrestrial irradiance, sun
zenith angle, and sun azimuth angle.

and then the parameter list could be one of two options:

dni : numeric
Direct normal irradiance, see :term:`dni`. [Wm⁻²]
dni : numeric
:term:`Direct Normal Irradiance<dni>` [Wm⁻²]

(note there was no full definition originally for this parameter)

My view (on the questions):
It would be ideal to link both, but I understand this would require more effort from contributors.
Undecided on glossary terms. Parameters need to be defined somewhere, either glossary, function docs, or both. Some parameters (e.g. dni in the linked example, #2234) aren't defined, in which case glossary terms in the parameter form is probably more important. I understand it was commented earlier that ideally functions should contain full definitions, so if we were to update all function docs to contain definitions then perhaps the glossary could contain 'normal text' terms. I'm not sure what would be best at this stage.

@cwhanse
Copy link
Member

cwhanse commented Sep 29, 2024

How is this rendered?

:term:`Direct Normal Irradiance<dni>`

Does it appear like this Direct Normal Irradiance ?

I think I prefer this style for both text and for parameter descriptions.

     dni : numeric 
         Direct normal irradiance, see :term:`dni`. [Wm⁻²] 

Where we have a term in the text, e.g., "direct normal irradiance" in the docstring for haydavies, I think it helps, but is optional, to add "see :term:dni" I would prefer readable text rather than complete crossreferences.

@cwhanse
Copy link
Member

cwhanse commented Sep 29, 2024

Do we want the glossary terms to be as written in the function or another form, e.g. expanded abbreviations?

For me, two primary use cases for the Glossary are:

  1. "pvlib uses this term e.g. dni_extra exactly what is that?"
  2. Answer this question: "I want to add code. What variable name should I use for < fill in a quantity here >?"

In a book, a Glossary is basically a dict; in the context of pvlib, the keys are the variable names, and the values are definitions or descriptions. If we did it differently (keys are acronyms of phrases) then I think we'd need to call it something other than Glossary, like "List of terms"

@RDaxini
Copy link
Contributor

RDaxini commented Sep 30, 2024

How is this rendered?

:term:`Direct Normal Irradiance<dni>`

Does it appear like this Direct Normal Irradiance ?

Correct --- see #2234 and this example function


In a book, a Glossary is basically a dict; in the context of pvlib, the keys are the variable names, and the values are definitions or descriptions. If we did it differently (keys are acronyms of phrases) then I think we'd need to call it something other than Glossary, like "List of terms"

Sorry, I didn't understand this part... do you mean "differently" with respect to a book? So we list variable names (which are sometimes initialisms/acronyms/abbreviations), and then title the page something other than "glossary"?

@cwhanse
Copy link
Member

cwhanse commented Sep 30, 2024

Sorry, I didn't understand this part...

I was trying to say that I think

  • 'dni': direct normal irradiance

is a more useful presentation than

  • Direct normal irradiance : dni

@RDaxini
Copy link
Contributor

RDaxini commented Sep 30, 2024

Thanks for the clarification

I will wait for a couple more weigh ins to confirm the plan of action here before I transfer all of the terms from the variables and symbols page over

@RDaxini
Copy link
Contributor

RDaxini commented Oct 8, 2024

Should I keep working on #2234? In its current form, is it in line with what people had in mind?

@cwhanse
Copy link
Member

cwhanse commented Oct 8, 2024

Should I keep working on #2234? In its current form, is it in line with what people had in mind?

I like the Glossary appearance, and the links from docstring to glossary. I wouldn't capitalize "Direct Normal Irradiance". Acronyms are capitalized, but the spelled-out words are only capitalized when they are proper nouns.

I'd say, press ahead.

@kandersolar
Copy link
Member Author

I like where #2234 is going too.

Another idea, which I suggest exploring after a working glossary page is merged: consider using sphinx-hoverxref to make these glossary cross-links even more useful.

@RDaxini
Copy link
Contributor

RDaxini commented Oct 9, 2024

Another idea, which I suggest exploring after a working glossary page is merged: consider using sphinx-hoverxref to make these glossary cross-links even more useful.

very cool! I will definitely look into that and try it out after we have a working glossary page merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants