Fix omission in snow_coverage_nrel #2292
Conversation
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think in general pvlib doesn't do quality checks of user input data unless bad data affects the output of the function.
pvlib/snow.py
Outdated
| Hourly snowfall above which snow coverage is set to the row's slant | ||
| height. [cm/hr] | ||
|
|
||
| Returns | ||
| ---------- | ||
| boolean: Series | ||
| Series |
There was a problem hiding this comment.
Should we have a name for the returned parameter? E.g., snow_coverage?
pvlib/snow.py
Outdated
| `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. |
There was a problem hiding this comment.
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.
pvlib/snow.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
| `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 |
pvlib/snow.py
Outdated
| 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. |
There was a problem hiding this comment.
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 zeroThere was a problem hiding this comment.
I think you are correct. The SAM implementation appears to iterate over timesteps.
There was a problem hiding this comment.
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.
docs/sphinx/source/referencefor 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`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.Additional scope if I can get clarity: describe whether
snowfallis left- or right-aligned.