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

[WIP] irradiance.py updates: glossary term links and units

Open RDaxini opened this issue 1 year ago • 1 comments

  • [X] Relates to: #2205, #2248, #2284, #2081
  • [X] I am familiar with the contributing guidelines
  • [ ] ~Tests added~
  • [ ] ~Updates entries in docs/sphinx/source/reference for API changes.~
  • [ ] Adds description and name entries in the appropriate "what's new" file in 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`).
  • [X] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [ ] Pull request is nearly complete and ready for detailed review.
  • [x] Maintainer: Appropriate GitHub Labels (including 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)

RDaxini avatar Nov 27 '24 04:11 RDaxini

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 irradiance.py might be a pain to review. Thoughts @kandersolar @AdamRJensen @cwhanse ?

RDaxini avatar Dec 09 '24 16:12 RDaxini

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.

RDaxini avatar Jun 30 '25 22:06 RDaxini

@cwhanse thank you for the meticulous review!

RDaxini avatar Jul 01 '25 03:07 RDaxini

Good to go, I think. Any further feedback?

RDaxini avatar Jul 29 '25 23:07 RDaxini

One question from my side: why in some functions you haven't written down the limits, while in some others you have?

E.g., in pvlib.irradiance.gti_dirint the zenith and azimuth are: image

While in pvlib.irradiance.perez_driesse: image

IoannisSifnaios avatar Jul 30 '25 18:07 IoannisSifnaios

@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?

RDaxini avatar Sep 05 '25 20:09 RDaxini

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 irradiance.aoi function works with any value of its input angles but only returns meaningful results for combinations of surface_tilt, surface_azimuth and solar position.

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.

cwhanse avatar Sep 05 '25 20:09 cwhanse

Thanks @cwhanse I'll update this PR soon. I haven't disappeared, just been busy lately.

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.

+1

RDaxini avatar Sep 05 '25 20:09 RDaxini

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 see :term:`solar_zenith` when I noticed that although the argument name is solar_zenith, the description states it is the apparent (non-refraction-correction) zenith angle. According to the nomenclature, these variables should be apparent_zenith. I think a separate PR that deprecates and updates the variable names would be appropriate rather than handling it here, if a change is required.

RDaxini avatar Sep 22 '25 21:09 RDaxini

I am available this week to try address any other comments, hopefully this should be good to go for 0.13.1.

RDaxini avatar Sep 22 '25 21:09 RDaxini

Thanks @RDaxini!

kandersolar avatar Sep 23 '25 14:09 kandersolar