BUG: fix kimber cleaning threshold and grace period off-by-one - #2814
BUG: fix kimber cleaning threshold and grace period off-by-one#2814dgowdaan-cmyk wants to merge 3 commits into
Conversation
- rainfall equal to cleaning_threshold now triggers cleaning - grace_period window now correctly includes the rain day itself - regenerated greensboro nowash/manwash expected test data - added tests for both cases Fixes pvlib#2796
|
@echedey-ls / @mikofski This is ready for review whenever you get a chance. Summary of the change:
Let me know if anything else is needed on my end. |
There was a problem hiding this comment.
Looks good to me, thx! @dgowdaan-cmyk please add your name to list of contributors. @cwhanse is this considered a breaking change? Anything special we need to do if it is?
|
@dgowdaan-cmyk can you fix the flake errors please? Thx! |
|
@mikofski Fixed the flake8 error (E127 continuation line indentation in test_soiling.py) and added my name to the contributors list. Let me know if anything else needs adjusting! |
|
@mikofski do you recall the source of the greensboro_ files used in the tests? |
It’s from tmy3 |
The expected output from |
|
@cwhanse, yes, it’s what’s in this example. Guess I didn’t do a very good job getting it first 🤣 |
Thanks, now I understand why the expected output files are changing. |
|
|
||
| # grace periods windows during which ground is assumed damp, so no soiling | ||
| grace_windows = rain_events.rolling(grace_period, closed='right').sum() > 0 | ||
| grace_windows = rain_events.rolling(grace_period, closed='both').sum() > 0 |
There was a problem hiding this comment.
This will not fix the off-by-one issue with grace periods. Grace periods are in days; closed='both' just adds the next timestamp to the grace period. In the case of the tests, it's adding one hour. rolling applies closed to the input data, not the output.
There was a problem hiding this comment.
Ahh yeah I see what you mean now @cwhanse
closed='both' doesn't solve it for daily grace periods.
We would need to shift the result forward by N-1 days instead.
I will take a look and push a fix for this. Thanks for catching it.
3 participants
Fixes #2796.
pvlib.soiling.kimberused a strict>comparison againstcleaning_threshold, so rainfall exactly equal to the threshold did nottrigger cleaning, contrary to Kimber (2006). It also had an off-by-one
error in the grace period window:
grace_period=1effectively providedzero days of protection because the rolling window excluded the rain
event's own day.
This PR:
Changes the comparison to
>=so rainfall equal to the threshold cleansthe panel.
Changes the grace period rolling window from
closed='right'toclosed='both'so the rain day itself is included, giving the fullgrace_perioddays of protection as documented.Regenerates the recorded expected test data
(
greensboro_kimber_soil_nowash.dat,greensboro_kimber_soil_manwash.dat)to reflect the corrected behavior.
Adds two new tests covering the threshold-equal case and the
grace_period=1 case.
Closes kimber grace and rain threshold implementation differences #2796
I am familiar with the contributing guidelines
I attest that all AI-generated material has been vetted for accuracy and is in compliance with the pvlib license
Tests added
Adds description and name entries in the appropriate "what's new" file in
docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:2796.New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.