Skip to content
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

Add model='gueymard2003' to get_relative_airmass #1655

Merged
merged 11 commits into from
Mar 9, 2023

Conversation

kandersolar
Copy link
Member

  • I am familiar with the contributing guidelines
  • Tests added
  • 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`).
  • New code is fully documented. Includes numpydoc 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.

This is the relative air mass model used in REST and REST2, as well as ASTM G222-21.

@kandersolar kandersolar added this to the 0.9.5 milestone Feb 4, 2023
* 'pickering2002' - See reference [6] -
requires apparent sun zenith
* 'gueymard2003' - See reference [7] -
requires true sun zenith
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put "true sun zenith" here because I found no mention of refraction correction, "apparent" etc in the reference, but it would be good if a reviewer could determine this with more confidence.

Copy link
Member

@adriesse adriesse Feb 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Chris Gueymard: "My formulas all use the apparent position--Chris"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chris also commented that the default should be a more modern model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chris also commented that the default should be a more modern model.

Does he have specific recommendations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implicitly, Gueymard 2003.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adriesse for checking!

Updated default: no objection from me, but I think it will have to wait for 0.10.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a separate issue. I think there could be some general discussion about defaults actually.

@kandersolar kandersolar mentioned this pull request Mar 2, 2023
12 tasks
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems to check out 😄

I would like to perhaps add a bit more context because the citation is not very explicit about the equation (the AM model is obscurely hidden in the appendix). Perhaps add the equation number in the citation and consider adding that it is what is used in the REST model.

The book chapter "Clear-Sky Radiation Models and Aerosol Effects" by Gueymard could be considered a good second reference. It provides the equation, lists a few sets of AM coefficients, and provides more context. The relevant part of the book chapter is:
image
image

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>
@AdamRJensen AdamRJensen merged commit bee329b into pvlib:main Mar 9, 2023
@kandersolar kandersolar deleted the rest_airmass branch March 9, 2023 16:46
@AdamRJensen
Copy link
Member

Thank you @kanderso-nrel 🚀

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.

4 participants