-
Notifications
You must be signed in to change notification settings - Fork 997
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
Change Variables and Symbols page into a Glossary of terms #2234
base: main
Are you sure you want to change the base?
Conversation
Irradiation is energy (e.g., Wh/m^2) whereas irradiance is power (e.g., W/m^2). I think it's better to use irradiance |
Co-Authored-By: Adam R. Jensen <39184289+adamrjensen@users.noreply.github.com>
Thoughts on retaining any of the following text from pvlib-python/docs/sphinx/source/user_guide/variables_style_rules.rst Lines 15 to 25 in cbe4cc5
|
I think its worth keeping the SodaPro references. We could drop the IEC 61724 links. |
A random thought, without any intentions: maybe instead of splitting and creating a whole new page, both of them could be put into the same one. |
If the old page is being removed, then these references need to be updated: $ git grep -n -F "variables_style_rules"
docs/sphinx/source/contributing/style_guide.rst:16::ref:`variables_style_rules`. We could be better about consistency.
docs/sphinx/source/index.rst:29:There is a :ref:`variable naming convention <variables_style_rules>` to
docs/sphinx/source/user_guide/index.rst:18: variables_style_rules
docs/sphinx/source/user_guide/variables_style_rules.rst:1:.. _variables_style_rules:
docs/sphinx/source/user_guide/variables_style_rules.rst:9: :file: ../../../../pvlib/data/variables_style_rules.csv
docs/sphinx/source/user_guide/weather_data.rst:86:into standard pvlib names (see :ref:`variables_style_rules`).
docs/sphinx/source/whatsnew/v0.3.0.txt:50: * :ref:`variables_style_rules` (:issue:`102`)
pvlib/iotools/midc.py:196: :ref:`variables_style_rules`. |
@kandersolar done, except for the 0.3.0 whatsnew entry. Anything needed there? Doesn't make sense to change the reference to "glossary", but should the :ref:`` be removed since the link won't work anyway (a warning is probably generated somewhere), or add something to say the page was removed in 11.2, or just leave it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RDaxini minor comments down below, feel free to discard any of them.
[v0.3.0 whatsnew entry] should the :ref:`` be removed since the link won't work anyway
I would strive to not have any warnings in the docs - that's noise when reviewing. Plain code formatting is good for me.
LGTM, loving these improvements on documentation 😋
Co-Authored-By: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>
The terms Nomenclature or Terminology seem to be more correct than Glossary, as the list includes both terms and symbols/variables. Glossary I believe should only contain terms/definitions. Terminology seems preferred to me as it's more easily understandable to non-native speakers. |
I don't mind "glossary", but I'm not opposed to changing the name. Just another thought: how about going back to the old name then, "Variables and Symbols"? I don't have a strong preference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall! I left one or two comments. Regarding the name of the section, I also agree that Nomenclature (assigning of a word or phrase to a particular object, event, or property) would be a more suitable term than Glossary (terms that are either newly introduced, uncommon, or specialized)
dni | ||
Direct normal irradiance [Wm⁻²]. Irradiance received per unit area by a | ||
surface perpendicular (normal) to the sun's rays that propagate in a | ||
straight line from the sun. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why DNI has an extensive explanation of what it is (including units) while the other irradiances don't? I kind of like the idea of reporting the unit that each term is (usually) measured in pvlib.
Average photon energy | ||
|
||
apparent_zenith | ||
Refraction-corrected solar zenith angle in degrees |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the unit is mentioned in text. Should it be [°] instead?
Broadband plane of array effective irradiance | ||
|
||
gamma_pdc | ||
Module temperature coefficient. Typically in units of 1/C. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again maybe try to be consistent on whether we report the units and how
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
I would be ok with either "Terminology" or "Variables and Symbols". |
I don't really like "Variables and Symbols"; to be precise it's more about "Common Parameters" - but I agree "Terminology" is good if it gets expanded to other terms in the future. An small side-note, the supposed link in https://pvlib-python--2234.org.readthedocs.build/en/2234/contributing/style_guide.html (first section "Code Style", second paragraph, second sentence) to the glossary is pointing to CPython's glossary: https://docs.python.org/3/glossary.html#glossary . It may be due to the crossreference at the top of the file missing an underscore at the beginning. |
Tests addedUpdates entries indocs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.The glossary would replace the variables and symbols page. This is a limited example. Links:
Glossary
Function (see parameters
surface_tilt
anddni
, and second paragraph in docstring)Points to discuss: see this comment
Note:
dni
andsurface_tilt
definition to show example enhancements (linked to the example function)Generally, I think adding/updating definitions (description, units, etc.) and adding/removing new/duplicate variables (#1421, #1253) probably requires a case-by-case discussion so I have not gone further here save for the two examples