Skip to content

Conversation

RDaxini
Copy link
Member

@RDaxini RDaxini commented Nov 27, 2024

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 RDaxini added this to the v0.11.2 milestone Nov 27, 2024
@RDaxini
Copy link
Member Author

RDaxini commented Dec 9, 2024

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 RDaxini modified the milestones: v0.11.2, v0.11.3 Dec 13, 2024
@RDaxini RDaxini mentioned this pull request Feb 25, 2025
8 tasks
@kandersolar kandersolar modified the milestones: v0.11.3, v0.11.4 Mar 14, 2025
@RDaxini RDaxini marked this pull request as ready for review June 30, 2025 22:14
@RDaxini
Copy link
Member Author

RDaxini commented Jun 30, 2025

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
Copy link
Member Author

RDaxini commented Jul 1, 2025

@cwhanse thank you for the meticulous review!

@RDaxini
Copy link
Member Author

RDaxini commented Jul 29, 2025

Good to go, I think. Any further feedback?

@IoannisSifnaios
Copy link
Contributor

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

@RDaxini
Copy link
Member Author

RDaxini commented Sep 5, 2025

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

@cwhanse
Copy link
Member

cwhanse commented Sep 5, 2025

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.

@RDaxini
Copy link
Member Author

RDaxini commented Sep 5, 2025

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 RDaxini mentioned this pull request Sep 19, 2025
9 tasks
@RDaxini
Copy link
Member Author

RDaxini commented Sep 22, 2025

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
Copy link
Member Author

RDaxini commented Sep 22, 2025

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

@kandersolar kandersolar merged commit d2f367e into pvlib:main Sep 23, 2025
4 checks passed
@kandersolar
Copy link
Member

Thanks @RDaxini!

@RDaxini RDaxini deleted the irradiance_updates branch September 23, 2025 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants