Improvements and correction to snow.loss_townsend #1653
Conversation
|
@kanderso-nrel curious what you think should be done with the current test. If it is checking against output from the original function, that output is wrong. The new tests have limited precision so may not serve as suitable regression checks. |
Isn't the original function's output correct except for the case when |
| snow_total, snow_events, surface_tilt, relative_humidity, temp_air, | ||
| poa_global, slant_height, lower_edge_height, string_factor) | ||
| actual = np.around(actual * 100) | ||
| assert np.allclose(expected, actual) |
There was a problem hiding this comment.
np.testing.assert_allclose is generally preferable over assert np.allclose because the former reports more detailed information about the differences
There was a problem hiding this comment.
LGTM. Thanks @cwhanse both for the PR and the behind-the-scenes work on this one.
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.Adds a lower bound to a term in the calculation of the ground interference term that prevents a negative squared distance from driving loss fractions to 1.0. Although the reference doesn't describe this limit, I consider its omission a bug in that results from the uncorrected function should be regarded as wrong.
Adds a new parameter, string_factor, that avoids loss fractions of 1.0 for systems with multiple strings across the slant height. This new parameter was added to the model by its author after publication of the reference.