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.2k stars 1k forks source link

Deprecate or better document `Location.tz` #1752

Open kandersolar opened 1 year ago

kandersolar commented 1 year ago

Location objects have a tz parameter and corresponding attribute. I think its purpose is confusing; unlike the other Location attributes (latitude, longitude, altitude), which get used in the helper methods, tz is not used anywhere in the Location class. I think it saw more use in the early versions of pvlib, but nowadays it seems to only get used in two places, both external to Location: forecast.py, which may be removed soon (https://github.com/pvlib/pvlib-python/issues/1735#issuecomment-1564605782), and tools.localize_to_utc, which itself is not used anywhere.

I suspect its existence might trip up people who assume that setting the correct tz in the Location is the path to getting correct solar positions.

I think we should at minimum do a better job of documenting what tz is and is not for, but I think I'd support deprecating and eventually removing it altogether.

cwhanse commented 1 year ago

My first reaction is "a location has a timezone". Maybe the aspiration was to offer a method that found the timezome from the position. I'm lukewarm about removing it. However, a Location doesn't (and won't, I think) have an associated datetime (or index) so I don't think keeping tz on Location is necessary.

I think at least one person was tripped up as you describe but I can't find the conversation now.

AdamRJensen commented 1 year ago

I've always found it very confusing and have seen people specify it and think it carries over to the Location.get_solarposition function.

I vote deprecating it.

wholmgren commented 1 year ago

Location.tz goes way back to the very earliest days of PVLIB_Python's pvl_makelocationstruct and was used by functions that accepted this struct and calculated solar position with it. A couple of examples:

https://github.com/pvlib/pvlib-python/blob/89cc1fcbd3eee11fe9ae179c9275a24c04820745/pvlib/pvl_ephemeris.py#LL105C28-L105C28

https://github.com/pvlib/pvlib-python/blob/89cc1fcbd3eee11fe9ae179c9275a24c04820745/pvlib/pvl_spa.py#L60

That pattern evolved into the Location class and its tz attribute, with inference of timezone when the user supplied an unlocalized DatetimeIndex to a function/method that calculated solar position. Somewhere around ~v0.4 we stopped using tz in this way and I think it became unused within pvlib except in forecast.py.

There was some discussion about optionally inferring the time zone using a library like timezone finder, but that didn't have much traction (and I don't support it today).

It would not surprise me if a small number of users find the attribute helpful for their own bookkeeping. So I lean towards first improving the documentation and if we're not happy with the results or if too many people are still confused then we can deprecate.