Fixed errors in soiling.hsu function and tests #977
Conversation
There was a problem hiding this comment.
Looks like mostly formatting corrections, click through the stickler-ci details in the list of checks. Do you want me to get a copy of the reference paper and double check the default values?
|
|
|
I think I have addressed most of the the stickler-ci details (except for the E203 ones which add readability). I pushed the changes. |
|
Heavy fingers in vi!
From: Cliff Hansen <[email protected]>
Reply-To: pvlib/pvlib-python <[email protected]>
Date: Wednesday, June 10, 2020 at 8:38 PM
To: pvlib/pvlib-python <[email protected]>
Cc: Joshua Stein <[email protected]>, Author <[email protected]>
Subject: [EXTERNAL] Re: [pvlib/pvlib-python] Fixed errors in soiling.hsu function and tests (#977)
@cwhanse commented on this pull request.
________________________________
In pvlib/tests/test_soiling.py<#977 (comment)>:
@@ -119,8 +110,8 @@ def test_hsu_defaults(rainfall_input, expected_output_1):
accumulation period.
"""
result = hsu(
- rainfall=rainfall_input, cleaning_threshold=0.5, tilt=0.0, pm2_5=1.0,
- pm10=2.0)
+ rainfall=rainfall_input, cleaning_threshold=0.5, tilt=0.0, i
stray i here is causing the test failures
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#977 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJES5CVEXE4EOIDQVJJ2ZLRWA7SDANCNFSM4NZ3PLTA>.
|
|
Thanks @jsstein |
| @@ -11,6 +11,7 @@ Enhancements | |||
|
|
|||
| Bug fixes | |||
| ~~~~~~~~~ | |||
| * Fixed unit and default value errors in :py:func:`pvlib.soiling.hsu`. (:pull:`XXX`) | |||
There was a problem hiding this comment.
Missed the PR number in the whatsnew entry
There was a problem hiding this comment.
I merged too early, thanks for taking a careful look
There was a problem hiding this comment.
I just submitted PR980 that addresses @kanderso-nrel 's comment about arbitrary time intervals. Next I will submit a demonstration of the hsu function showing that it matches one of the figures in the original JPV paper.
|
|
||
| # accumulate rainfall into periods for comparison with threshold | ||
| accum_rain = rainfall.rolling(rain_accum_period, closed='right').sum() | ||
| # cleaning is True for intervals with rainfall greater than threshold | ||
| cleaning_times = accum_rain.index[accum_rain >= cleaning_threshold] | ||
|
|
||
| horiz_mass_rate = pm2_5 * depo_veloc['2_5']\ | ||
| + np.maximum(pm10 - pm2_5, 0.) * depo_veloc['10'] | ||
| + np.maximum(pm10 - pm2_5, 0.) * depo_veloc['10'] * 3600 |
There was a problem hiding this comment.
Sorry if I'm thinking about this wrong, but I want to raise two points about this calculation:
- Is it correct to hardcode the 3600? Shouldn't it be based on the datetime index's interval? I'm not seeing any documentation requiring the input timeseries to be hourly.
- Regardless of whatever value that timescale coefficient takes, shouldn't it apply to both particulate sizes? Right now it only affects pm10.
There was a problem hiding this comment.
@kanderson-nrel: I agree that I should take another look at the hardcoding of 3600. About your second point, let me look more carefully at the Matlab version and see how it deals with it.
docs/sphinx/source/api.rstfor 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`).