-
Notifications
You must be signed in to change notification settings - Fork 1.1k
add max_airmass=12 kwarg to disc #626
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
cwhanse
left a comment
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.
I'm OK with this implementation. Exposing max_airmass in disc and diriint is fine.
| Default value (`max_airmass=12`) is consistent with polynomial fit in | ||
| original paper describing the model. Output of | ||
| :py:func:`pvlib.irradiance.dirint` and `:py:func:`pvlib.irradiance.dirindex`, | ||
| are different when `min_cos_zenith` and `max_zenith` kwargs are used. |
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.
This second sentence is not clear to me. Are you saying that the two functions produce different output with the same kwarg values, or that each function's output is different if the kwarg is set different from its default?
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.
The outputs of each of these functions will be different than they were before if the user set the kwargs to more permissive values.
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.
to be clear, this is only because of the change in disc and not because I've directly changed anything in these other functions.
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.
Perhaps "Changing the value(s) of min_cos_zenith or max_zenith kwargs will change the output of :py:func:pvlib.irradiance.dirint and :py:func:pvlib.irradiance.dirindex`.
Yes. My preference would be to expose it in all three functions, but I don't feel strongly about it. |
|
I moved the airmass limit into |
docs/sphinx/source/api.rstfor API changes.docs/sphinx/source/whatsnewfile for all changes.@cwhanse @adriesse ready for review.