Horizon coefficient should be allowed to go negative in the Perez transposition model

[cpr-chas]

Horizon coefficient should be allowed to go negative in the Perez transposition model

Link to the original paper: https://www.osti.gov/servlets/purl/7024029
Section III.2 states this explicitly for the horizon component:
"(2) The horizon brightening coefficient, F2, is negative for overcast and low E occurrences -- indicative of brightening of the zenithal region of the sky for these conditions. This becomes positive past intermediate conditions and increases substantially with clearness."

Note that this change also impacts results from the DIRINT model (used to generate DNI from GHI).

• Closes The Perez diffuse model should not be forcing the horizon coefficient up to zero #1238
• I am familiar with the contributing guidelines
• Tests added
• Updates entries to docs/sphinx/source/api.rst for API changes.
• Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

[cpr-chas]

It appears that I have violated a number of formatting requirements, so I'll work on getting those addressed.

[wholmgren]

Thanks @cpr-chas! We're not too strict about these particular formatting requirements in the test files when they occur in array input:

E203 whitespace before ','
E231 missing whitespace after ','
E202 whitespace before ']'

It's already a pain to add comprehensive array values to test against and we don't want to make it too much of a burden. Other requirements like trailing whitespace at the end of a line, indents, and space around = are still important for long term readability.

[cpr-chas]

Thanks Will. I already had a lot of those changes in progress so went ahead and finished it up, but it looks like I just ended up creating (or revealing) others. Is there any way to run this manually on my local clone?

[wholmgren]

pip install flake8 then git diff upstream/master -u -- "*.py" | flake8 --diff. This assumes that you have a git remote named upstream that points to pvlib/pvlib-python. If your local master branch is up to date then you can delete the upstream/.

[cpr-chas]

Perfect. Thanks Will!

[cwhanse]

I have checked the new test against the Matlab code. The results agree.

[mikofski]

Do we need a note to explain how this will affect existing results? Is it only during sunrise, low irradiance?

[cpr-chas]

I expect it to affect primarily low irradiance, overcast conditions, but is not limited to sunrise/sunset.
Any recommendations on what to put into "what's new"? I assume that's the last piece I'm missing and that I don't need to update any api or code documentation.

[cwhanse]

I expect it to affect primarily low irradiance, overcast conditions, but is not limited to sunrise/sunset.
Any recommendations on what to put into "what's new"? I assume that's the last piece I'm missing and that I don't need to update any api or code documentation.

Correct, just add a note to whatsnew under "Bug fixes".

Suggestion: "Corrected an error in :py:func:~pvlib.irradiance.perez where the horizon irradiance component was prevented from taking negative values. Negative values are intentional according to the original publication. Changes in output are
expected to be small and primarily occur at low irradiance conditions."

[cpr-chas]

I expect it to affect primarily low irradiance, overcast conditions, but is not limited to sunrise/sunset.
Any recommendations on what to put into "what's new"? I assume that's the last piece I'm missing and that I don't need to update any api or code documentation.

Correct, just add a note to whatsnew under "Bug fixes".

Suggestion: "Corrected an error in :py:func:~pvlib.irradiance.perez where the horizon irradiance component was prevented from taking negative values. Negative values are intentional according to the original publication. Changes in output are
expected to be small and primarily occur at low irradiance conditions."

Done. I didn't see anything after the 0.9.0 release so I added it there under the assumption that this release isn't closed yet. Let me know if that's not the right place. Thanks!

[cpr-chas]

@pvlib/maintainer - does this still need to be assigned official reviewers? I'm not sure what the process is here since several people have obviously looked at the change already, but I do see that other PRs have explicit reviewers listed and I wasn't sure if I had to specifically request a review.

[mikofski]

Hi @cpr-chas I'll try to look through for syntax today asap unless someone else beats me to it. I believe @cwhanse has already approved it verbally for consistency with pvlib-MATLAB and Perez's original intent based on the literature. thanks for your patience, bringing the issue up, and the fix! 😄

[mikofski]

Syntax all looks great. Thanks! @cwhanse I vote to merge. Please do the honors if you agree.

[cwhanse]

thanks @cpr-chas

[cwhanse]

@wholmgren in case you have projects that depend on the perez function, I'll wait to merge until you approve.

[wholmgren]

looks good to me. Thanks @cpr-chas!