irradiance.py updates: glossary term links and units #2311
irradiance.py updates: glossary term links and units #2311kandersolar merged 33 commits intopvlib:mainfrom
Conversation
|
I could modify the scope of this PR if we want to see some of the changes implemented in 11.2. I don't think these changes are urgent though so I'm also happy to keep working on it and merge the completed version in 11.3. Not sure what reviewers would prefer though--- on second thoughts many small revisions covering the entire |
Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
…-python into irradiance_updates
|
I went for the format the seems to have the most in favour and is what is currently outlined in our contributing guidelines: link to nomenclature, period, units, no period All parameters that have an entry on the nomenclature page and require a unit should now have both. The parameter description structure for all parameters should also now be consistent. |
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
|
@cwhanse thank you for the meticulous review! |
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
|
Good to go, I think. Any further feedback? |
|
@IoannisSifnaios good point, thanks. I think it's just my error. I don't think any of these functions enforce the limit. I feel like there was a discussion somewhere else about whether these limits should be included in docstrings but I cannot remember where... @cwhanse might have been a part of it, can you advise? |
|
Value limits in docstrings: much of that text was ported to python from the original Matlab code. There have been two, sometimes competing objectives for value limits: 1) describe the range of values that the code can process, and 2) describe limits that are reasonable for the physical device or process being modeled. Violating #1 would give an error or an incorrect result; violating #2 gives a value that's not meaningful (that's not the same as being incorrect). For example, the It is my opinion that only code-imposed value limits should be present in the definition of a parameter. If there are also reasonable ranges of values, I would put those in a Note section. For definitions in the Glossary, I don't think we should include either type of range, since they probably depend on the use of that quantity in a function. |
|
Thanks @cwhanse
+1 |
|
I have resolved the issue regarding the angle restraints. I did not evaluate every documented upper/lower limit, there may still be some that are unrelated to the changes in this PR but could benefit from clarification as whether they are advisory or enforced requirements. I think that can be handled separately. I also removed some of the |
|
I am available this week to try address any other comments, hopefully this should be good to go for 0.13.1. |
|
Thanks @RDaxini! |


Tests addedUpdates entries indocs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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.Updating the units to superscript and linking key terms to their glossary page definitions.
Follow up PR(s): add some of these key terms into the glossary, enhance existing glossary term definitions (units and explanation)
Note: we can now view definition tooltips by hovering the cursor over the linked glossary term (context: #2290)