Skip to content

Townsend snow #1251

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

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Snow
snow.coverage_nrel
snow.fully_covered_nrel
snow.dc_loss_nrel
snow.loss_townsend

Soiling
-------
Expand Down
4 changes: 4 additions & 0 deletions docs/sphinx/source/whatsnew/v0.9.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ Enhancements
* Added ``map_variables`` option to :func:`~pvlib.iotools.read_crn` (:pull:`1368`)
* Added :py:func:`pvlib.temperature.prilliman` for modeling cell temperature
at short time steps (:issue:`1081`, :pull:`1391`)
* Added Townsend Powers Snow loss model in :py:func:`pvlib.snow`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Added Townsend Powers Snow loss model in :py:func:`pvlib.snow`
* Added Townsend-Powers monthly snow loss model: :py:func:`pvlib.snow.townsend`

(:issue:`1246`, :pull:`1251`)

Bug fixes
~~~~~~~~~
Expand Down Expand Up @@ -83,3 +85,5 @@ Contributors
* Jack Kelly (:ghuser:`JackKelly`)
* Somasree Majumder(:ghuser:`soma2000-lang`)
* Naman Priyadarshi (:ghuser:`Naman-Priyadarshi`)
* Abhishek Parikh (:ghuser:`abhisheksparikh`)

107 changes: 106 additions & 1 deletion pvlib/snow.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import numpy as np
import pandas as pd
from pvlib.tools import sind
from pvlib.tools import sind, cosd, tand


def _time_delta_in_hours(times):
Expand Down Expand Up @@ -185,3 +185,108 @@ def dc_loss_nrel(snow_coverage, num_strings):
Available at https://www.nrel.gov/docs/fy18osti/67399.pdf
'''
return np.ceil(snow_coverage * num_strings) / num_strings


def _townsend_Se(S, N):
'''
Calculates effective snow for a given month based upon the total snowfall
received in a month in inches and the number of events where snowfall is
greater than 1 inch

Parameters
----------
S : numeric
Snowfall in inches received in a month [in]

N: numeric
Number of snowfall events with snowfall > 1" [-]
Copy link
Member

Choose a reason for hiding this comment

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

1" here too


Returns
-------
effective_snowfall : numeric
Effective snowfall as defined in the townsend model

References
----------
.. [1] Townsend, Tim & Powers, Loren. (2011). Photovoltaics and snow: An
update from two winters of measurements in the SIERRA. Conference
Record of the IEEE Photovoltaic Specialists Conference.
003231-003236. :doi:`10.1109/PVSC.2011.6186627`
Available at https://www.researchgate.net/publication/261042016_Photovoltaics_and_snow_An_update_from_two_winters_of_measurements_in_the_SIERRA

''' # noqa: E501
return(np.where(N > 0, 0.5 * S * (1 + 1/N), 0))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return(np.where(N > 0, 0.5 * S * (1 + 1/N), 0))
return np.where(N > 0, 0.5 * S * (1 + 1/N), 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'm surprised stickler doesn't complain about this.



def loss_townsend(snow_total, snow_events, surface_tilt, relative_humidity,
Copy link
Member

Choose a reason for hiding this comment

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

"_townsend" or "_townsend_powers"? Often a paper will be cited as "X and Y" if only two authors, "X et al" if 3 or more. Not committed to that pattern here, just providing context. @mikofski?

Copy link
Member

Choose a reason for hiding this comment

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

It's a tough call, but I believe most folks already refer to this model as the "Townsend snow model"

temp_air, poa_global, slant_height, lower_edge_drop_height,
angle_of_repose=40):
'''
Calculates monthly snow loss based on a generalized monthly snow loss model
discussed in [1]_.
Comment on lines +225 to +226
Copy link
Member

Choose a reason for hiding this comment

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

"Calculates monthly snow loss based on the Townsend monthly snow loss model [1]_." or Townsend-Powers.


Parameters
----------
snow_total : numeric
Copy link
Member

Choose a reason for hiding this comment

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

Our current definition of numeric includes scalars (e.g. float), which I don't think makes sense here. Would array-like make sense? See https://pvlib-python.readthedocs.io/en/stable/contributing.html#documentation

Inches of snow received in the current month. Referred as S in the
paper [in]

snow_events : numeric
Number of snowfall events with snowfall > 1". Referred as N in the
Copy link
Member

Choose a reason for hiding this comment

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

Is this > 1" requirement in the reference? I'm not seeing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope - it isn't, hence removing it. I guess it is better to leave that up to the user (to define what a snow event is).

paper [-]

surface_tilt : numeric
Array surface_tilt [deg]

relative_humidity : numeric
Relative humidity [%]

temp_air : numeric
Ambient temperature [°C]
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth explicitly using the word "monthly" for all the monthly inputs, and "average/total/etc" as appropriate. Temperature is monthly average, and I'm not seeing it in the reference but I guess RH is monthly average as well?


poa_global : numeric
Plane of array insolation [kWh/m2/month]

slant_height : float
Row length in the slanted plane of array dimension [in]

lower_edge_drop_height : float
Drop height from array edge to ground [in]
Copy link
Member

Choose a reason for hiding this comment

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

To discuss: what units should snow_total, slant_height, and lower_edge_drop_height use? I propose cm, m, m respectively, all for consistency with other pvlib functions.

Copy link
Contributor Author

@abhisheksparikh abhisheksparikh Mar 22, 2022

Choose a reason for hiding this comment

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

I prefer metric system over everything else :)

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 can change slant_height and lower_edge_drop_height inputs to m. However, I think, we should leave snow_total in in because that's what is available in most of the weather sources. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Other than a general reluctance to introduce imperial units to pvlib, the main reason I suggested cm is because snow.fully_covered_nrel takes snowfall in cm. Do non-US datasets use inches? I've only ever used snowfall data from the GHCN, which I think reports in inches.

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK asking users to do some unit conversions to keep consistent units for similar functions in pvlib

Copy link
Member

Choose a reason for hiding this comment

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

NOAA NCEI provides monthly climate normals in both metric and imperial units.

I agree that sticking with metric units in pvlib is preferable. I think sticking with a single unit system within a single function is a must.


P : float
Copy link
Member

Choose a reason for hiding this comment

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

This P slipped through the cracks :)

piled snow angle, assumed to stabilize at 40° , the midpoint of
25°-55° avalanching slope angles [deg]

Returns
-------
loss : numeric
Average monthly DC capacity loss due to snow coverage [%]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Average monthly DC capacity loss due to snow coverage [%]
Average monthly DC capacity loss fraction due to snow coverage


Copy link
Member

Choose a reason for hiding this comment

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

Let's add a Notes section here pointing out that this model has not been validated for tracking arrays, but the reference suggests using the maximum rotation angle in place of surface_tilt.

References
----------
.. [1] Townsend, Tim & Powers, Loren. (2011). Photovoltaics and snow: An
update from two winters of measurements in the SIERRA. Conference
Record of the IEEE Photovoltaic Specialists Conference.
003231-003236. 10.1109/PVSC.2011.6186627.
Available at https://www.researchgate.net/publication/261042016_Photovoltaics_and_snow_An_update_from_two_winters_of_measurements_in_the_SIERRA
''' # noqa: E501

C1 = 5.7e04
C2 = 0.51

snow_total_prev = np.roll(snow_total, 1)
snow_events_prev = np.roll(snow_events, 1)

Se = _townsend_Se(snow_total, snow_events)
Se_prev = _townsend_Se(snow_total_prev, snow_events_prev)

Se_weighted = 1/3 * Se_prev + 2/3 * Se
Comment on lines +280 to +283
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Se = _townsend_Se(snow_total, snow_events)
Se_prev = _townsend_Se(snow_total_prev, snow_events_prev)
Se_weighted = 1/3 * Se_prev + 2/3 * Se
effective_snow = _townsend_Se(snow_total, snow_events)
effective_snow_prev = _townsend_Se(snow_total_prev, snow_events_prev)
effective_snow_weighted = 1/3 * effective_snow_prev + 2/3 * effective_snow

gamma = (slant_height * Se_weighted * cosd(surface_tilt)) / \
(np.clip((lower_edge_drop_height**2 - Se_weighted**2), a_min=0.01,
a_max=None) / 2 / tand(angle_of_repose))
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 disagree with the spirit of having a lower bound of 0.01, but is there anything special about that value other than being small but nonzero? May be worth a comment explaining it.

Copy link
Contributor Author

@abhisheksparikh abhisheksparikh Mar 22, 2022

Choose a reason for hiding this comment

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

I think a_min is set at 0.01 to prevent gamma from blowing up.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I was more wondering why specifically 0.01 is the lower limit (why not 0.1, 0.001, or any other small nonzero value?). I guess I was more curious than anything else -- the specific lower bound, so long as it's <<1, doesn't seem to matter much. Feel free to ignore this comment :)

Copy link
Member

@mikofski mikofski Mar 22, 2022

Choose a reason for hiding this comment

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

A common small value is eps or sometimes EPS**(1/3) but I don’t know the mathematics behind why. EPS is the smallest number that can represent on the runtime machine. See np.finfo
https://numpy.org/doc/stable/reference/generated/numpy.finfo.html

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it seems the danger is having negative gamma, which upstream could result in a performance gain (from negative GIT in Eq. 3). To me, clipping gamma to be positive (not zero) is reasonable. It is not clear if gamma should have an upper bound, I haven't untangled the model equations enough to determine,

Copy link
Member

Choose a reason for hiding this comment

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

This line is pretty difficult for me to parse - can we break it up in a couple of assignments or be more aggressive with line breaks?

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it seems the danger is having negative gamma, which upstream could result in a performance gain (from negative GIT in Eq. 3). To me, clipping gamma to be positive (not zero) is reasonable.

If I understand correctly... I mostly agree, but why can't gamma be 0? Shouldn't that be expected when snow fall is 0? I think mathematically gamma will blow up when both the snow fall is 0 and the drop height goes to 0. But I think in practice python will evaluate 0 / 0 and return nan. Assuming we want to avoid that... Drop height should always be greater than 0, even if just by 1 mm, so maybe that's the point to insert our judgement into the code.

Copy link
Member

@wholmgren wholmgren Mar 22, 2022

Choose a reason for hiding this comment

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

My comment above is not correct. Sorry. Gamma will blow up as effective snow fall approaches the drop height. Beyond that, gamma is large and negative, eventually approaching 0 again.

image

The ground interference term goes negative when gamma is more negative than ln(c2) ~= -0.67.

Comment on lines +284 to +286
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gamma = (slant_height * Se_weighted * cosd(surface_tilt)) / \
(np.clip((lower_edge_drop_height**2 - Se_weighted**2), a_min=0.01,
a_max=None) / 2 / tand(angle_of_repose))
drop_height_clipped = np.maximum(lower_edge_drop_height, 0.01)
gamma = (
slant_height
* effective_snow_weighted
* cosd(surface_tilt))
/ (drop_height_clipped**2 - effective_snow_weighted **2)
* 2 # eqn 5 would benefit from another set of parentheses but I believe
* tand(angle_of_repose) # the 2tan is effectively in the numerator of gamma
)


GIT = 1 - C2 * np.exp(-gamma)
Copy link
Member

Choose a reason for hiding this comment

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

I'd also change this to ground_interference_term and use more lines in the equation below.

loss = (C1 * Se_weighted * (cosd(surface_tilt))**2 * GIT *
relative_humidity / (temp_air+273.15)**2 / poa_global**0.67) / 100
Copy link
Member

Choose a reason for hiding this comment

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

Good catch that temperature is in Kelvin 👍


return loss
29 changes: 29 additions & 0 deletions pvlib/tests/test_snow.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,32 @@ def test_dc_loss_nrel():
expected = pd.Series([1, 1, .5, .625, .25, .5, 0])
actual = snow.dc_loss_nrel(snow_coverage, num_strings)
assert_series_equal(expected, actual)


def test__townsend_Se():
S = np.array([10, 10, 5, 1, 0, 0, 0, 0, 0, 0, 5, 10])
N = np.array([2, 2, 1, 0, 0, 0, 0, 0, 0, 0, 2, 3])
expected = np.array([7.5, 7.5, 5, 0, 0, 0, 0, 0, 0, 0, 3.75, 6.66666667])
actual = snow._townsend_Se(S, N)
np.testing.assert_allclose(expected, actual, rtol=1e-07)


def test_loss_townsend():
Copy link
Member

Choose a reason for hiding this comment

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

Question for @cwhanse: referring to #1393 (comment), should we have a policy of always testing both array and Series for array-like parameters?

Copy link
Member

Choose a reason for hiding this comment

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

should we have a policy of always testing both array and Series for array-like parameters?

I think so

Copy link
Member

Choose a reason for hiding this comment

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

I'd say it's good practice but importance and extent depends on the model and implementation. I'm usually more concerned that we test for compatibility with scalars in "float, array, series" situations because I think it's easier for regressions to slip in with scalars (e.g. by introducing masking).

snow_total = np.array([10, 10, 5, 1, 0, 0, 0, 0, 0, 0, 5, 10])
snow_events = np.array([2, 2, 1, 0, 0, 0, 0, 0, 0, 0, 2, 3])
surface_tilt = 20
relative_humidity = np.array([80, 80, 80, 80, 80, 80, 80, 80, 80, 80,
80, 80])
temp_air = np.array([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0])
poa_global = np.array([350, 350, 350, 350, 350, 350, 350, 350, 350, 350,
350, 350])
angle_of_repose = 40
slant_height = 100
lower_edge_drop_height = 10
expected = np.array([0.07696253, 0.07992262, 0.06216201, 0.01715392, 0, 0,
0, 0, 0, 0, 0.02643821, 0.06068194])
actual = snow.loss_townsend(snow_total, snow_events, surface_tilt,
relative_humidity, temp_air,
poa_global, slant_height,
lower_edge_drop_height, angle_of_repose)
np.testing.assert_allclose(expected, actual, rtol=1e-05)