Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
mikofski merged 7 commits into from
May 14, 2021
Merged

Better handling of missing inputs in get_total_irradiance, get_sky_diffuse #1225

mikofski merged 7 commits into from
May 14, 2021

Conversation

@cwhanse
Copy link
Member

@cwhanse cwhanse commented May 10, 2021 •
edited
Loading

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.

@cwhanse cwhanse added this to the 0.9.0 milestone May 10, 2021
@cwhanse cwhanse mentioned this pull request May 10, 2021
Closed
24 tasks
@mikofski
Copy link
Member

mikofski commented May 10, 2021

Thanks @cwhanse I like this solution. Sorry I dropped the ball on this.

@cwhanse cwhanse changed the title Better handle missing inputs in get_total_irradiance, get_sky_diffuse Better handling of missing inputs in get_total_irradiance, get_sky_diffuse May 10, 2021
@cwhanse cwhanse requested a review from mikofski May 11, 2021 16:18
mikofski
mikofski approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@mikofski mikofski left a 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.

mikofski
mikofski reviewed May 12, 2021
View reviewed changes
mikofski
mikofski reviewed May 12, 2021
View reviewed changes
mikofski
mikofski reviewed May 12, 2021
View reviewed changes
@mikofski
Copy link
Member

mikofski commented May 12, 2021

Thanks @cwhanse !

wholmgren
wholmgren approved these changes May 13, 2021
View reviewed changes
mikofski
mikofski reviewed May 13, 2021
View reviewed changes
@mikofski mikofski mentioned this pull request May 13, 2021
Open
@mikofski
Copy link
Member

mikofski commented May 14, 2021

Thanks Cliff. Can I push the button when the tests are done?

@cwhanse
Copy link
Member Author

cwhanse commented May 14, 2021

Push the green button - yes. Not the red one.

@mikofski mikofski merged commit 01a0840 into pvlib:master May 14, 2021
@mikofski
Copy link
Member

mikofski commented May 14, 2021

🚀

@kandersolar kandersolar mentioned this pull request Sep 18, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikofski mikofski mikofski approved these changes

@wholmgren wholmgren wholmgren approved these changes

Assignees

No one assigned

Labels

documentation enhancement

Projects

None yet

Milestone

0.9.0

Development

Successfully merging this pull request may close these issues.

Improve docstring or behavior for irradiance.get_total_irradiance and irradiance.get_sky_diffuse

3 participants

@cwhanse @mikofski @wholmgren