Skip to content
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.

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

Fix omission in snow_coverage_nrel #2292

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
39 changes: 30 additions & 9 deletions pvlib/snow.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,30 +13,36 @@ def _time_delta_in_hours(times):
return delta.dt.total_seconds().div(3600)


def fully_covered_nrel(snowfall, threshold_snowfall=1.):
def fully_covered_nrel(snowfall, snow_depth=None, threshold_snowfall=1.):
'''
Calculates the timesteps when the row's slant height is fully covered
by snow.

Parameters
----------
snowfall : Series
Accumulated snowfall in each time period [cm]

threshold_snowfall : float, default 1.0
snowfall: Series
Accumulated snowfall in each time period. [cm]
snow_depth: Series, optional
Snow depth on the ground at the beginning of each time period.
Must have the same index as `snowfall`. [cm]
threshold_snowfall: float, default 1.0
Hourly snowfall above which snow coverage is set to the row's slant
height. [cm/hr]

Returns
----------
boolean: Series
Series
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have a name for the returned parameter? E.g., snow_coverage?

True where the snowfall exceeds the defined threshold to fully cover
the panel.

Notes
-----
Implements the model described in [1]_ with minor improvements in [2]_.

`snow_depth` is used to set coverage=0 when no snow is present on the
ground. This check is described in [2]_ as needed for systems with
low tilt angle.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this explanation fits better the coverage_nrel function, right? I guess here it should be something like:
snow_depth is used to set snow_covered=False when no snow is present on the
ground. This check is described in [2]_ as needed for systems with
low tilt angle.


References
----------
.. [1] Marion, B.; Schaefer, R.; Caine, H.; Sanchez, G. (2013).
Expand All @@ -56,11 +62,15 @@ def fully_covered_nrel(snowfall, threshold_snowfall=1.):
hourly_snow_rate.iloc[0] = snowfall.iloc[0] / timedelta
else: # can't infer frequency from index
hourly_snow_rate.iloc[0] = 0 # replaces NaN
return hourly_snow_rate > threshold_snowfall
covered = (hourly_snow_rate > threshold_snowfall)
# no coverage when no snow on the ground
if snow_depth is not None:
covered = covered & (snow_depth > 0.)
return covered


def coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt,
initial_coverage=0, threshold_snowfall=1.,
snow_depth=None, initial_coverage=0, threshold_snowfall=1.,
can_slide_coefficient=-80., slide_amount_coefficient=0.197):
'''
Calculates the fraction of the slant height of a row of modules covered by
Expand All @@ -82,6 +92,9 @@ def coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt,
surface_tilt : numeric
Tilt of module's from horizontal, e.g. surface facing up = 0,
surface facing horizon = 90. [degrees]
snow_depth : Series, optional
Snow depth on the ground at the beginning of each time period.
Must have the same index as `snowfall`. [cm]
initial_coverage : float, default 0
Fraction of row's slant height that is covered with snow at the
beginning of the simulation. [unitless]
Expand All @@ -106,6 +119,10 @@ def coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt,
In [1]_, `can_slide_coefficient` is termed `m`, and the value of
`slide_amount_coefficient` is given in tenths of a module's slant height.

`snow_depth` is used to set coverage=0 when no snow is present on the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`snow_depth` is used to set coverage=0 when no snow is present on the
`snow_depth` is used to set snow_coverage=0 when no snow is present on the

ground. This check is described in [2]_ as needed for systems with
low tilt angle.

References
----------
.. [1] Marion, B.; Schaefer, R.; Caine, H.; Sanchez, G. (2013).
Expand All @@ -117,7 +134,7 @@ def coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt,
'''

# find times with new snowfall
new_snowfall = fully_covered_nrel(snowfall, threshold_snowfall)
new_snowfall = fully_covered_nrel(snowfall, snow_depth, threshold_snowfall)

# set up output Series
snow_coverage = pd.Series(np.nan, index=poa_irradiance.index)
Expand All @@ -143,6 +160,10 @@ def coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt,
snow_coverage.ffill(inplace=True)
snow_coverage -= cumulative_sliding

if snow_depth is not None:
# no coverage when there's no snow on the ground
# described in [2] to avoid non-sliding snow for low-tilt systems.
snow_coverage[snow_depth <= 0] = 0.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this check can be applied at the end like this. In cases where the check takes effect (snow_depth = 0 --> coverage = 0), that effect should propagate forward in time and influence values for future timestamps. As it is now, I don't think this code allows that.

See for example this input:

times = pd.date_range("2019-01-01", freq="h", periods=4)
snowfall = pd.Series([10, 0, 0, 0.1], index=times)  # last value is below threshold_snowfall
snow_depth = pd.Series([10, 5, 0, 0.1], index=times)
poa_irradiance = pd.Series(100, index=times)
temp_air = pd.Series(-1, index=times)
surface_tilt = 10

coverage_nrel(snowfall, poa_irradiance, temp_air, surface_tilt, snow_depth)

# output:
2019-01-01 00:00:00    1.000000
2019-01-01 01:00:00    0.965791
2019-01-01 02:00:00    0.000000
2019-01-01 03:00:00    0.897374  # this value doesn't make sense, should be zero

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are correct. The SAM implementation appears to iterate over timesteps.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the SAM algorithm, these lines set coverage to full if both of two conditions are met:

	if ((snowDepth - previousDepth) >= deltaThreshold*dt && snowDepth >= depthThreshold){
		coverage = 1;
	}

So change in snow depth exceeds deltaThreshold (*dt converts to hourly), and snowDepth exceeds a different threshold so the snow is "sticking" around.

# clean up periods where row is completely uncovered
return snow_coverage.clip(lower=0)

Expand Down
33 changes: 33 additions & 0 deletions pvlib/tests/test_snow.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,18 @@ def test_fully_covered_nrel():
assert_series_equal(expected, fully_covered)


def test_fully_covered_nrel_with_snow_depth():
dt = pd.date_range(start="2019-1-1 12:00:00", end="2019-1-1 18:00:00",
freq='1h')
snowfall_data = pd.Series([1, 5, .6, 4, .23, -5, 19], index=dt)
snow_depth = pd.Series([0., 1, 6, 6.6, 10.6, 10., -2], index=dt)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if a negative snow_depth is given, it is handled as 0, right? Maybe there should be a warning about this. Or should an error be raised instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in general pvlib doesn't do quality checks of user input data unless bad data affects the output of the function.

expected = pd.Series([False, True, False, True, False, False, False],
index=dt)
fully_covered = snow.fully_covered_nrel(snowfall_data,
snow_depth=snow_depth)
assert_series_equal(expected, fully_covered)


def test_coverage_nrel_hourly():
surface_tilt = 45
slide_amount_coefficient = 0.197
Expand All @@ -38,6 +50,27 @@ def test_coverage_nrel_hourly():
assert_series_equal(expected, snow_coverage)


def test_coverage_nrel_hourly_with_snow_depth():
surface_tilt = 45
slide_amount_coefficient = 0.197
dt = pd.date_range(start="2019-1-1 10:00:00", end="2019-1-1 17:00:00",
freq='1h')
poa_irradiance = pd.Series([400, 200, 100, 1234, 134, 982, 100, 100],
index=dt)
temp_air = pd.Series([10, 2, 10, 1234, 34, 982, 10, 10], index=dt)
snowfall_data = pd.Series([1, .5, .6, .4, .23, -5, .1, .1], index=dt)
snow_depth = pd.Series([1, 1, 1, 1, 0, 1, 0, .1], index=dt)
snow_coverage = snow.coverage_nrel(
snowfall_data, poa_irradiance, temp_air, surface_tilt,
snow_depth=snow_depth, threshold_snowfall=0.6)

slide_amt = slide_amount_coefficient * sind(surface_tilt)
covered = 1.0 - slide_amt * np.array([0, 1, 2, 3, 4, 5, 6, 7])
expected = pd.Series(covered, index=dt)
expected[snow_depth <= 0] = 0
assert_series_equal(expected, snow_coverage)


def test_coverage_nrel_subhourly():
surface_tilt = 45
slide_amount_coefficient = 0.197
Expand Down
Loading