add orgill hollands decomposition #1730
Conversation
pvlib/irradiance.py
Outdated
| bolland | ||
|
|
||
| """ | ||
| dni_extra = get_extra_radiation(datetime_or_doy) |
There was a problem hiding this comment.
Do we want to add an argument dni_extra=None and calculate here only if dni_extra is None? Orgill and Hollands didn't specify the solar constant they used. It may be useful to provide users an option besides the pvlib default.
|
Hi @nicorie, the next step here is for you to push new commits with updates according to Cliff's review above. Just a heads up in case the PR review process isn't clear :) |
correct spelling to 'boland' Co-authored-by: Cliff Hansen <[email protected]>
correct citation formating Co-authored-by: Cliff Hansen <[email protected]>
correct citation formatting Co-authored-by: Cliff Hansen <[email protected]>
correct spelling 'boland' Co-authored-by: Cliff Hansen <[email protected]>
adding dni_extra argument per cliff's suggestion.
@kandersolar thank you for the heads up. @cwhanse I pushed several commits per your comments. please check. |
|
@nicorie sometimes git has issues when new code is similar to nearby, older code - such is the case here with Two space indent on the 2nd and later lines of a reference. We need to describe the function output. Need to add a few tests. We could compute output for some short, assumed input using the old PVLib for Matlab function, but with this simple calculation I think it's better to just do it in a spreadsheet. No need to include the spreadsheet as a data file if you choose that way. |
|
@nicorie I've gone ahead and fixed the merge conflict that occurred and the reference formatting Cliff was mentioning. For these changes to show up on your local branch you need to click "fetch origin" in GitHub Desktop: The next step is for you to add some tests. 🥳 The easiest thing is to mimic the test for the Erbs function since the two functions are very similar. Don't hesitate to ask questions! |
Update docs
|
@nicorie I'm happy to add the unit tests if you'd like me to do that. |
Adding a test
fixing the perilous outcome of copy/pasting the test_boland function.
|
@cwhanse thanks but @AdamRJensen was kind enough to give me a crash course on tests. I added a test_orgill_hollands function to test_irradiance.py. It is a replica of test_boland, but the dni and dhi columns in the expected output array are updated to reflect the orgill and hollands model. The kt values were selected based on what was used previously for the test_erbs and test_boland function, but these kt values still cover all three piece wise functions of the orgill and hollands model. I ran the pytest in my anaconda prompt. I get 6 warnings (that I don't understand) and 1 pass. |
|
I think the only thing missing now is a note in the what's new page; can you add an entry for this new function under
Those are unrelated to your test and can be ignored for this PR. Sorry for the clutter! |
Adding Orgill Hollands model to list of enhancements.
There was a problem hiding this comment.
LGTM. Thanks @nicorie and congratulations on your first merged PR to pvlib-python :)
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.This PR adds the Orgill and Hollands decomposition model (https://doi.org/10.1016/0038-092X(77)90006-8) to the irradiance module.