Skip to content

Fixed errors in soiling.hsu function and tests #977

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.

Sign up for GitHub

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 5 commits into from
Jun 11, 2020
Merged

Fixed errors in soiling.hsu function and tests #977

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8c3758
bug fixes to hsu soiling function. Addresses (#970)
jsstein May 20, 2020
3a77400
Made changes to the soiling tests Addresses (#970)
jsstein Jun 9, 2020
bff444f
update what's new documentation
jsstein Jun 9, 2020
f412ddb
Fixed stickler-ci issues in test_soiling.py
jsstein Jun 11, 2020
db6bb16
Fixed errant character in test
jsstein Jun 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/sphinx/source/whatsnew/v0.8.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Enhancements

Bug fixes
~~~~~~~~~
* Fixed unit and default value errors in :py:func:`pvlib.soiling.hsu`. (:pull:`XXX`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed the PR number in the whatsnew entry

Copy link
Member

@cwhanse cwhanse Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged too early, thanks for taking a careful look

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Testing
~~~~~~~
Expand All @@ -32,3 +33,4 @@ Contributors
* Cliff Hansen (:ghuser:`cwhanse`)
* Kevin Anderson (:ghuser:`kanderso-nrel`)
* Mark Mikofski (:ghuser:`mikofski`)
* Joshua S. Stein (:ghuser:`jsstein`)
6 changes: 3 additions & 3 deletions pvlib/soiling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def hsu(rainfall, cleaning_threshold, tilt, pm2_5, pm10,
Concentration of airborne particulate matter (PM) with
aerodynamicdiameter less than 10 microns. [g/m^3]

depo_veloc : dict, default {'2_5': 0.4, '10': 0.09}
depo_veloc : dict, default {'2_5': 0.0009, '10': 0.004}
Deposition or settling velocity of particulates. [m/s]

rain_accum_period : Timedelta, default 1 hour
Expand Down Expand Up @@ -69,15 +69,15 @@ def hsu(rainfall, cleaning_threshold, tilt, pm2_5, pm10,

# never use mutable input arguments
if depo_veloc is None:
depo_veloc = {'2_5': 0.004, '10': 0.0009}
depo_veloc = {'2_5': 0.0009, '10': 0.004}

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tilted_mass_rate = horiz_mass_rate * cosd(tilt) # assuming no rain

# tms -> tilt_mass_rate
Expand Down
67 changes: 29 additions & 38 deletions pvlib/tests/test_soiling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,48 +19,40 @@ def expected_output():
end=pd.Timestamp(2019, 1, 1, 23, 59, 0), freq='1h')

expected_no_cleaning = pd.Series(
data=[0.884980357535360, 0.806308930084762, 0.749974647038078,
0.711804155175089, 0.687489866078621, 0.672927554408964,
0.664714899337491, 0.660345851212099, 0.658149551658860,
0.657104593968981, 0.656633344364056, 0.656431630729954,
0.656349579062171, 0.656317825078228, 0.656306121502393,
0.656302009396500, 0.656300630853678, 0.656300189543417,
0.656300054532516, 0.656300015031680, 0.656300003971846,
0.656300001006533, 0.656300000244750, 0.656300000057132],
data=[0.97230454, 0.95036146, 0.93039061, 0.91177978, 0.89427556,
0.8777455 , 0.86211038, 0.84731759, 0.83332881, 0.82011354,
0.80764549, 0.79590056, 0.78485556, 0.77448749, 0.76477312,
0.75568883, 0.74721046, 0.73931338, 0.73197253, 0.72516253,
0.7188578 , 0.71303268, 0.7076616 , 0.70271919],
index=dt)

return expected_no_cleaning


@pytest.fixture
def expected_output_1():
return np.array([
0.99927224, 0.99869067, 0.99815393, 0.99764437, 1.0,
0.99927224, 0.99869067, 0.99815393, 1.0, 1.0,
0.99927224, 0.99869067, 0.99815393, 0.99764437, 0.99715412,
0.99667873, 0.99621536, 0.99576203, 0.99531731, 0.9948801,
0.99444954, 0.99402494, 0.99360572, 0.99319142])

dt = pd.date_range(start=pd.Timestamp(2019, 1, 1, 0, 0, 0),
end=pd.Timestamp(2019, 1, 1, 23, 59, 0), freq='1h')
expected_output_1 = pd.Series(
data=[0.9872406 , 0.97706269, 0.96769693, 0.95884032, 1.,
0.9872406 , 0.97706269, 0.96769693, 1. , 1. ,
0.9872406 , 0.97706269, 0.96769693, 0.95884032, 0.95036001,
0.94218263, 0.93426236, 0.92656836, 0.91907873, 0.91177728,
0.9046517 , 0.89769238, 0.89089165, 0.88424329],
index=dt)
return expected_output_1

@pytest.fixture
def expected_output_2(expected_output):
# Sample output (calculated manually)
def expected_output_2():
dt = pd.date_range(start=pd.Timestamp(2019, 1, 1, 0, 0, 0),
end=pd.Timestamp(2019, 1, 1, 23, 59, 0), freq='1h')
expected_output_2 = pd.Series(
data = [0.97229869, 0.95035106, 0.93037619, 0.91176175, 1.,
1. , 1. , 0.97229869, 1. , 1. ,
1. , 1. , 0.97229869, 0.95035106, 0.93037619,
0.91176175, 0.89425431, 1. , 1. , 1. ,
1. , 0.97229869, 0.95035106, 0.93037619],
index=dt)

expected_no_cleaning = expected_output

expected = pd.Series(index=dt, dtype='float64')
expected[dt[:4]] = expected_no_cleaning[dt[:4]]
expected[dt[4:7]] = 1.
expected[dt[7]] = expected_no_cleaning[dt[0]]
expected[dt[8:12]] = 1.
expected[dt[12:17]] = expected_no_cleaning[dt[:5]]
expected[dt[17:21]] = 1.
expected[dt[21:]] = expected_no_cleaning[:3]

return expected

return expected_output_2

@pytest.fixture
def rainfall_input():
Expand All @@ -81,7 +73,7 @@ def test_hsu_no_cleaning(rainfall_input, expected_output):
rainfall = rainfall_input
pm2_5 = 1.0
pm10 = 2.0
depo_veloc = {'2_5': 1.0, '10': 1.0}
depo_veloc = {'2_5': 1.0e-5, '10': 1.0e-4}
tilt = 0.
expected_no_cleaning = expected_output

Expand All @@ -99,16 +91,15 @@ def test_hsu(rainfall_input, expected_output_2):
rainfall = rainfall_input
pm2_5 = 1.0
pm10 = 2.0
depo_veloc = {'2_5': 1.0, '10': 1.0}
depo_veloc = {'2_5': 1.0e-4, '10': 1.0e-4}
tilt = 0.
expected = expected_output_2

# three cleaning events at 4:00-6:00, 8:00-11:00, and 17:00-20:00
result = hsu(rainfall=rainfall, cleaning_threshold=0.5, tilt=tilt,
pm2_5=pm2_5, pm10=pm10, depo_veloc=depo_veloc,
rain_accum_period=pd.Timedelta('3h'))

assert_series_equal(result, expected)
assert_series_equal(result, expected_output_2)


@requires_scipy
Expand All @@ -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, pm2_5=1.0e-2,
pm10=2.0e-2)
assert np.allclose(result.values, expected_output_1)


Expand Down