Skip to content

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

Merged
merged 6 commits into from
Dec 11, 2018
Merged

add max_airmass=12 kwarg to disc #626

merged 6 commits into from
Dec 11, 2018

Conversation

wholmgren
Copy link
Member

@wholmgren wholmgren commented Nov 26, 2018

  • Closes DISC Knc out of range #450
  • I am familiar with the contributing guidelines.
  • Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
  • Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
  • New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
  • Pull request is nearly complete and ready for detailed review.

@cwhanse @adriesse ready for review.

@wholmgren wholmgren added this to the 0.6.1 milestone Nov 26, 2018
Copy link
Member

@cwhanse cwhanse left a 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.
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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`.

@wholmgren
Copy link
Member Author

Thanks for the review @cwhanse. Would you be ok with not exposing the max_airmass kwarg in any of the functions? That is @adriesse's preference. If we keep the kwarg in disc I will leave it to others to do the work of exposing it in dirint and dirindex, if desired.

@cwhanse
Copy link
Member

cwhanse commented Nov 26, 2018

Would you be ok with not exposing the max_airmass kwarg in any of the functions?

Yes. My preference would be to expose it in all three functions, but I don't feel strongly about it.

@wholmgren
Copy link
Member Author

I moved the airmass limit into _disc_kn so that it's also applied in gti_dirint. The test failure is unrelated. I think it's ready to merge.

@cwhanse cwhanse merged commit d6b22c3 into pvlib:master Dec 11, 2018
@wholmgren wholmgren deleted the discamlim branch December 11, 2018 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants