-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add reference for Gueymard 93 relative airmass model #2424
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
Conversation
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.
Since we're working on the references, let's add some underscores? This should hyperlink the numbers to their respective citation in the list at the bottom of the page.
(I can only suggest changes to lines that were changed or next to those that were changes, but I think we should update the entire page in this PR)
pvlib/atmosphere.py
Outdated
@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'): | |||
requires true sun zenith | |||
* 'kastenyoung1989' (default) - See reference [3] - | |||
requires apparent sun zenith | |||
* 'gueymard1993' - See reference [4] - | |||
* 'gueymard1993' - See references [4] and [9] - |
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.
* 'gueymard1993' - See references [4] and [9] - | |
* 'gueymard1993' - See references [4]_ and [9]_ - |
pvlib/atmosphere.py
Outdated
@@ -174,6 +174,8 @@ def get_relative_airmass(zenith, model='kastenyoung1989'): | |||
other models use true (not refraction-adjusted) zenith angle. Apparent | |||
zenith angles should be calculated at sea level. | |||
|
|||
Comparison among several models is reported in [10]. |
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.
Comparison among several models is reported in [10]. | |
Comparison among several models is reported in [10]_. |
pvlib/atmosphere.py
Outdated
@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'): | |||
requires true sun zenith | |||
* 'kastenyoung1989' (default) - See reference [3] - |
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.
* 'kastenyoung1989' (default) - See reference [3] - | |
* 'kastenyoung1989' (default) - See reference [3]_ - |
pvlib/atmosphere.py
Outdated
@@ -153,7 +153,7 @@ def get_relative_airmass(zenith, model='kastenyoung1989'): | |||
requires true sun zenith | |||
* 'kastenyoung1989' (default) - See reference [3] - | |||
requires apparent sun zenith | |||
* 'gueymard1993' - See reference [4] - | |||
* 'gueymard1993' - See references [4] and [9] - | |||
requires apparent sun zenith | |||
* 'young1994' - See reference [5] - |
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.
* 'young1994' - See reference [5] - | |
* 'young1994' - See reference [5]_ - |
pvlib/atmosphere.py
Outdated
* 'kasten1966' - See [1]_ - requires apparent sun zenith | ||
* 'youngirvine1967' - See [2]_ - requires true sun zenith | ||
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith | ||
* 'gueymard1993' - See [4]_, [9]_ - requires apparent sun zenith | ||
* 'young1994' - See [5]_ - requires true sun zenith | ||
* 'pickering2002' - See [6]_ - requires apparent sun zenith | ||
* 'gueymard2003' - See [7]_, [8]_ - requires apparent sun zenith |
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.
Nitpicking, but should we adjust the order? [9] -> [5] and so on. I missed this earlier, just noticed it now.
(reference list would need to change accordingly)
* 'kasten1966' - See [1]_ - requires apparent sun zenith | |
* 'youngirvine1967' - See [2]_ - requires true sun zenith | |
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith | |
* 'gueymard1993' - See [4]_, [9]_ - requires apparent sun zenith | |
* 'young1994' - See [5]_ - requires true sun zenith | |
* 'pickering2002' - See [6]_ - requires apparent sun zenith | |
* 'gueymard2003' - See [7]_, [8]_ - requires apparent sun zenith | |
* 'kasten1966' - See [1]_ - requires apparent sun zenith | |
* 'youngirvine1967' - See [2]_ - requires true sun zenith | |
* 'kastenyoung1989' (default) - See [3]_ - requires apparent sun zenith | |
* 'gueymard1993' - See [4]_, [5]_ - requires apparent sun zenith | |
* 'young1994' - See [6]_ - requires true sun zenith | |
* 'pickering2002' - See [7]_ - requires apparent sun zenith | |
* 'gueymard2003' - See [8]_, [9]_ - requires apparent sun zenith |
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.
LGTM!
|
||
Documentation | ||
~~~~~~~~~~~~~ | ||
* Add a supporting reference to :py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`) |
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.
* Add a supporting reference to :py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`) | |
* Add a supporting bibliographic reference to model ``gueymard1993`` in | |
:py:func:`pvlib.atmosphere.get_relative_airmass` (:issue:`2390`, :pull:`2424`) |
Clarification for users out of context.
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.
I thought just "reference" was okay without "bibliographic". Adding the model ``gueymard1993`` in
seems reasonable though
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.
Agreed. I sometimes think of code references, but given this piece of description is in the Docs section, you are completely right.
pvlib/atmosphere.py
Outdated
In: Polo, J., Martín-Pomares, L., Sanfilippo, A. (eds) Solar Resources | ||
Mapping. Green Energy and Technology. Springer, Cham. | ||
:doi:`10.1007/978-3-319-97484-2_5` | ||
|
||
.. [9] Matthew J. Reno, Clifford W. Hansen and Joshua S. Stein, "Global | ||
.. [10] Matthew J. Reno, Clifford W. Hansen and Joshua S. Stein, "Global | ||
Horizontal Irradiance Clear Sky Models: Implementation and Analysis" | ||
Sandia Report, (2012). |
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.
Sandia Report, (2012). | |
Sandia Report, (2012). | |
:doi:`10.2172/1039404` |
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.
I can't add DOIs for the other references but here they are:
1: 11681/5671
2: 10.1086/110366
3: 10.1364/AO.28.004735
4: 10.1016/0038-092X(93)90074-X
Thanks @RDaxini @AdamRJensen your comments motivated me to update the formatting to the IEEE convention. |
pvlib/atmosphere.py
Outdated
|
||
.. [2] A. T. Young and W. M. Irvine, "Multicolor Photoelectric | ||
Photometry of the Brighter Planets," The Astronomical Journal, vol. | ||
72, pp. 945-950, 1967. | ||
:doi:`10.2172/110366` |
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.
hmm... this doi did not work for me. Try:
:doi:`10.2172/110366` | |
:doi:`10.1086/110366` |
pvlib/atmosphere.py
Outdated
|
||
.. [7] Keith A. Pickering. "The Ancient Star Catalog". DIO 12:1, 20, | ||
.. [7] Keith A. Pickering, "The Southern Limits of the Ancient Star Catalog | ||
and the Commentary fo Hipparchos," DIO, vol. 12, pp. 3-27, Sept. 2002. |
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.
fo needs to be of (for some reason it won't let me suggest it)
Could consider adding the url: http://dioi.org/jc01.pdf
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.
LGTM
Thanks everyone for the reviews! |
[ ] Tests added[ ] Updates 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`
).[ ] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.remote-data
) and Milestone are assigned to the Pull Request and linked Issue.