Skip to content

Deprecate or better document Location.tz #1752

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

Open
kandersolar opened this issue May 29, 2023 · 3 comments
Open

Deprecate or better document Location.tz #1752

kandersolar opened this issue May 29, 2023 · 3 comments

Comments

@kandersolar
Copy link
Member

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 (#1735 (comment)), 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
Copy link
Member

cwhanse commented May 29, 2023

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
Copy link
Member

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
Copy link
Member

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

Timeshifted=Time.shift(abs(Location.TZ),freq='H') #This will work with a timezone unaware dataset

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants