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

Klucher diffuse sky docstring inconsistency #1405

Closed
Tracked by #1424
adriesse opened this issue Feb 14, 2022 · 3 comments · Fixed by #2192
Closed
Tracked by #1424

Klucher diffuse sky docstring inconsistency #1405

adriesse opened this issue Feb 14, 2022 · 3 comments · Fixed by #2192
Milestone

Comments

@adriesse
Copy link
Member

The equation for F' should contain DHI rather than Id0.

Versions:

  • pvlib.__version__: 0.9 and earlier
@cwhanse cwhanse added this to the 0.9.1 milestone Feb 14, 2022
@cwhanse
Copy link
Member

cwhanse commented Feb 14, 2022

I'm sure that Loutzenhiser's notation is I_{d} sky diffuse on a surface tilted at d, and I_{d0} is sky diffuse on a horizontal surface. But that's not helpful documentation here.

To do with this: add inline citations, punctuation, units in brackets, define theta, theta_z, and beta.

Maybe jettison the whole second paragraph that reiterates the inputs, and incorrectly adds extraterrestrial irradiance.

To discuss: remove "must be >=0", or change to "should be >=0". The code doesn't require this restriction, although the model certainly isn't valid outside that range. My view: model constraints on input (should be's) would be better placed in the description or in Notes, and code restrictions (must be's) in the parameter descriptions.

@adriesse adriesse changed the title Klucher diffuse sky docstring error Klucher diffuse sky docstring inconsistency Feb 14, 2022
@cwhanse cwhanse mentioned this issue Mar 14, 2022
14 tasks
@kandersolar
Copy link
Member

Seems like several of those items apply to the other sky diffuse model docstrings as well.

Also, does it make sense for the Loutzenhiser reference to be listed first? Seems like it ought to be a secondary reference if it's included at all.

@cwhanse
Copy link
Member

cwhanse commented Mar 16, 2022

Also, does it make sense for the Loutzenhiser reference to be listed first? Seems like it ought to be a secondary reference if it's included at all.

Although Loutzenhiser is a survey and comparison among models, it more clearly defines several models than do the source papers. That's why we chose it as the baseline reference.

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.

3 participants