pvlib / pvlib-python

A set of documented functions for simulating the performance of photovoltaic energy systems.
https://pvlib-python.readthedocs.io
BSD 3-Clause "New" or "Revised" License
1.16k stars 979 forks source link

Consider changing `pvlib.snow` to expect precipitation in mm instead of cm #1792

Closed kandersolar closed 1 year ago

kandersolar commented 1 year ago

It would make a whole lot of sense for iotools functions to return rainfall in the units that pvlib.soiling expects and snowfall in the units that pvlib.snow expects. However, those modules use different units: rainfall in mm in pvlib.soiling and snowfall in cm in pvlib.snow. Thus we are faced with choosing the lesser of two evils: either we return multiple units from iotools 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.

But ask me again tomorrow and I might have a different opinion. (https://github.com/pvlib/pvlib-python/pull/1767#pullrequestreview-1501412825)

@wholmgren now that it's tomorrow, has your opinion changed? ;)

wholmgren commented 1 year ago

Not yet 😀

cwhanse commented 1 year ago

I have preferred to think of iotools as functions that read data, and that return the data provided by the source, with minimal manipulation. That way, the functions are more easily re-used outside of pvlib.

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 pvlib_units=True

kandersolar commented 1 year ago

It seems to me that mapping variable names and mapping units ought to go hand in hand. read_tmy3(..., map_variables=True) returning data with pvlib names but not pvlib units (e.g. its current behavior of pressure in mbar) seems like something that many will stumble over.

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.

cwhanse commented 1 year ago

It seems to me that mapping variable names and mapping units ought to go hand in hand. read_tmy3(..., map_variables=True) returning data with pvlib names but not pvlib units (e.g. its current behavior of pressure in mbar) seems like something that many will stumble over.

I like the idea of mapping both names and units to pvlib with map_variables=True

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'm OK with the unit change but I'm unclear how that simplifies anyone's workflow, if snowfall data are usually in cm.

kandersolar commented 1 year ago

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 pvlib.snow and have it be the only kid on the block that uses mm for snow.

I'd rather keep using cm in pvlib.snow, and return snow data in cm in #1767. Different units for different types of precipitation doesn't seem so bad to me.

wholmgren commented 1 year ago

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.