-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Consider changing pvlib.snow
to expect precipitation in mm instead of cm
#1792
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
Comments
Not yet 😀 |
I have preferred to think of I'm also not a big fan of changing units in model inputs from those in the source paper, but in some cases its necessary (e.g., snow.townsend) and it certainly is convenient, so I'm not strongly opposed to doing this. We have added a kwarg that coerces variable names so that the returned data is easy to use inside pvlib. Not renaming would be a major pain for pvlib users, but I note that we don't rename without offering a way to prevent renaming. I don't think changing units (e.g. mm to cm) is nearly the burden that renaming can be. If we try to keep both uses in mind for iotools functions (return data as read, and return data normalized to pvlib units), could that be controlled with a second kwarg? I would be OK with the default being |
It seems to me that mapping variable names and mapping units ought to go hand in hand. However, for the purposes of this issue, the immediate question is whether we should switch the canonical pvlib snowfall/snow depth unit from cm to mm. |
I like the idea of mapping both names and units to pvlib with
I'm OK with the unit change but I'm unclear how that simplifies anyone's workflow, if snowfall data are usually in cm. |
A few minutes of searching for snowfall and snow depth datasets are turning up numbers in cm or inches. After more thought, I've become more opposed to this change. The models (neither our implementations nor the original references) take snowfall in mm, and I'm not immediately finding snow datasets with units of mm. The two reasons mentioned in #1767 (units consistent with rainfall; less proliferation of units overall) don't seem compelling enough (to me, at least) to warrant a no-deprecation break to I'd rather keep using cm in |
I took a quick look at vendor API docs. Solcast API docs say snow depth in cm. SolarAnywhere says snow depth in m. Solargis only has a snow-water equivalent in kg/m^2. |
It would make a whole lot of sense for
iotools
functions to return rainfall in the units thatpvlib.soiling
expects and snowfall in the units thatpvlib.snow
expects. However, those modules use different units: rainfall in mm inpvlib.soiling
and snowfall in cm inpvlib.snow
. Thus we are faced with choosing the lesser of two evils: either we return multiple units fromiotools
or we burden the user with converting units before running models.Discussion in #1767 showed tentative support for settling on mm for both types of precipitation and changing the expected units in
pvlib.snow
. This would be a change that is not easy (or perhaps not possible) to robustly deprecate, so if we are going to make that change, I think it makes sense to squeeze it into 0.10.0. But I'd like to see more support for this change first.@wholmgren now that it's tomorrow, has your opinion changed? ;)
The text was updated successfully, but these errors were encountered: