-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Better handling of missing inputs in get_total_irradiance, get_sky_diffuse #1225
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
Thanks @cwhanse I like this solution. Sorry I dropped the ball on this. |
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.
some minor typos I caught like get_rel_airmass
is in pvl.atmosphere
not irradiance
and grounddiffuse
is now called get_ground_diffuse
, a missing space, I copied the Raises section from get_sky_diffuse
to get_total_irrad
, and I fixed a typo in the what's new, get_irrad
was listed 2x and the PR was off by 1. I also added syntax highlighting everywhere I thought was necessary, but I wasn't sure what the convention is for keyword arguments, if they're supposed to be single or double backticks. Sorry if I messed that up.
Thanks @cwhanse ! |
Thanks Cliff. Can I push the button when the tests are done? |
Push the green button - yes. Not the red one. |
🚀 |
Updates entries todocs/sphinx/source/api.rst
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`
).Missing
airmass
can be calculated.Calculating missing
dni_extra
requires having a datetime index. Since the functions are not constrained to input type Series, I suggest that pvlib raise an informative ValueError and let the user handle it.