Fix2448 #2485
Fix2448 #2485
Conversation
- Add detailed definitions for solar angles with range constraints - Clarify pvlib's east-of-north convention for azimuth angles - Add cross-references between related terms using :term: directive - Add coordinate system conventions for latitude/longitude - Enhance existing angle definitions with usage notes and examples This commit addresses issue pvlib#2448 by migrating angle definitions and conventions from parameter descriptions to the nomenclature page. (cherry picked from commit 16435c7)
- Created test_solar_angles.py to verify solar angle calculations - Tests zenith, azimuth, and elevation angles for different times of day - Uses New York City as example location on spring equinox - Verifies angles are within expected ranges and follow correct patterns (cherry picked from commit f202c5d)
…s/test_solarposition.py - Deleted the standalone test_solar_angles.py file. - Added a new test function, test_solar_angles_spring_equinox, to tests/test_solarposition.py. - The new test verifies solar angles for New York City on the spring equinox, ensuring angles are within expected ranges and follow correct patterns. (cherry picked from commit 9963629)
| @@ -22,16 +22,23 @@ There is a convention on consistent variable names throughout the library: | |||
|
|
|||
| aoi | |||
| Angle of incidence. Angle between the surface normal vector and the | |||
| vector pointing towards the sun’s center | |||
| vector pointing towards the sun's center. Must be >=0 and <=180 degrees. | |||
There was a problem hiding this comment.
Must be >=0 and <=180 degrees.
I'm mildly opposed to including this range description. I don't think that pvlib functions require that constraint. It is certainly not checked for the user. And I don't think that pvlib promises useful results if aoi falls in that range.
Is the intent here to inform a user of the usual range for this quantity? If so, and we agree to add a statement about "typical" ranges, then I don't think we should use the verb "must".
@pvlib/pvlib-maintainer please weigh in.
There was a problem hiding this comment.
I agree with Cliff. I don’t think this constraint is necessary. A scan of the irradiance.py module yields the following functions with aoi as an argument:
poa_componentshas a note that says if AOI is negative or >90° then components would be zero…
pvlib-python/pvlib/irradiance.py
Line 500 in a27c686
… but this is incorrect according to the code. It will only be zero if < -90° or >90°
pvlib-python/pvlib/irradiance.py
Line 504 in a27c686
gti_dirinthas two subfunctions that depend on whether AOI is greater than or less than 90° which I think is a bug 🐞 and probable should read ifabs(AOI) < 90
pvlib-python/pvlib/irradiance.py
Line 2384 in a27c686
If not a bug 🐞 then it seems to me that AOI is assumed to be in the range of [0, 180] but then it’s still a a bug 🐞 because AFAICT this is never enforced
iam.pyalso uses AOI for example in ASHRAE, it clearly limitsabs(AOI) < 90
Line 85 in a27c686
Anyway, I suggest continue searching through the code, and find out what it actually says because it seems to me like there may be different assumptions
There was a problem hiding this comment.
@OmarBahamida to let this PR move ahead without getting really involved, let's leave out any statements about a range of values.
| Azimuth angle of the surface | ||
| Azimuth angle of the surface in degrees East of North. Must be >=0 and <=360. | ||
| The convention is defined as degrees east (clockwise) of north. This is pvlib's | ||
| convention; other tools may use different conventions. For example, North = 0°, |
There was a problem hiding this comment.
I suggest moving the sentence, "For example, North...", before the sentence, "This is pvlib's convention...".
As it is, I interpret the example as an example of some other tool's convention, not as an example of pvlib's convention.
|
One quick note @OmarBahamida , that also relates to your other open PR #2486; when you opened the pull request, a template with a checklist must have been already there. Please add it again to the top of your pull request message body and mark items as required (not all may apply). Pull request template
<!-- Thank you for your contribution! The following items must be addressed before the code can be merged. Please don't hesitate to ask for help if you're unsure of how to accomplish any of the items. Feel free to remove checklist items that are not relevant to your change. -->
- [ ] Closes #xxxx
- [ ] I am familiar with the [contributing guidelines](https://pvlib-python.readthedocs.io/en/latest/contributing/index.html)
- [ ] Tests added
- [ ] Updates entries in [`docs/sphinx/source/reference`](https://github.com/pvlib/pvlib-python/blob/main/docs/sphinx/source/reference) for API changes.
- [ ] Adds description and name entries in the appropriate "what's new" file in [`docs/sphinx/source/whatsnew`](https://github.com/pvlib/pvlib-python/tree/main/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` ``).
- [ ] New code is fully documented. Includes [numpydoc](https://numpydoc.readthedocs.io/en/latest/format.html) compliant docstrings, examples, and comments where necessary.
- [ ] Pull request is nearly complete and ready for detailed review.
- [ ] Maintainer: Appropriate GitHub Labels (including `remote-data`) and Milestone are assigned to the Pull Request and linked Issue.
<!-- Brief description of the problem and proposed solution (if not already fully described in the issue linked to above): --> |
This commit addresses issue #2448 by migrating angle definitions and conventions from parameter descriptions to the nomenclature page