Skip to content

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

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

Closed
cwhanse opened this issue Apr 2, 2020 · 5 comments · Fixed by #1225
Milestone

Comments

@cwhanse
Copy link
Member

cwhanse commented Apr 2, 2020

pvlib.irradiance.get_total_irradiance accepts kwargs dni_extra and airmass, both default to None. However, values for these kwargs are required for several of the irradiance transposition models.

See discussion here

Docstring should specify when dni_extra and airmass are required, and which airmass is appropriate for each model.

Could also test for kwarg values if e.g. model=='perez'

@cwhanse cwhanse changed the title Improve docstring for irradiance.get_total_irradiance Improve docstring for irradiance.get_total_irradiance and irradiance.get_sky_diffuse Apr 2, 2020
@kandersolar
Copy link
Member

Consider also specifying it is relative airmass.

@CameronTStark CameronTStark added this to the 0.7.3 milestone Apr 3, 2020
@cwhanse cwhanse modified the milestones: 0.7.3, 0.8.0 May 19, 2020
@wholmgren
Copy link
Member

This came up again in @mikofski's thread here.

@mikofski proposed calculating values if needed and not provided.

@kanderso-nrel proposed a couple of solutions to provide more informative error messages: 1. hard coding the failure modes in get_sky_diffuse and 2. using a decorator to communicate the failure modes.

And repeating my take from the thread:

I'd rather not introduce the complexity of decorators to the lower level pvlib functions to solve this problem.

Ideas that I support:

  • calculate if not provided (Mark's original idea)
  • reraising a more informative message.
  • require all of the arguments in get_sky_diffuse and get_total_irradiance regardless of whether or not they're used.
  • remove the arguments from get_sky_diffuse and get_total_irradiance and do the calculation if it's needed
  • remove get_sky_diffuse and get_total_irradiance. I'm not convinced they're a net positive for the library. (let's try to fix it before throwing up our hands)

Does someone want to tackle this in 0.8.0 or should we kick it down the road?

@mikofski
Copy link
Member

mikofski commented Sep 5, 2020

I think it's tempting to add it in v0.8, but I'm in favor of freezing features now and pushing out the release sooner with the features we already have queued. It's been a while, and I think we should deploy more often with less features per release. I believe this will make it easier to blame issues and get more testing done on new features and fixes faster.

@mikofski
Copy link
Member

mikofski commented Sep 5, 2020

I'll volunteer to take this up for v0.8.1, since I was the complainer.

@wholmgren wholmgren modified the milestones: 0.8.0, 0.8.1 Sep 5, 2020
@wholmgren wholmgren changed the title Improve docstring for irradiance.get_total_irradiance and irradiance.get_sky_diffuse Improve docstring or behavior for irradiance.get_total_irradiance and irradiance.get_sky_diffuse Sep 6, 2020
@wholmgren
Copy link
Member

PVSystem.get_irradiance has some relevant shim code:

# not needed for all models, but this is easier
if dni_extra is None:
dni_extra = irradiance.get_extra_radiation(solar_zenith.index)
if airmass is None:
airmass = atmosphere.get_relative_airmass(solar_zenith)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants