Add tests for time conversions in tools package #2341

markcampanelli · 2024-12-23T04:59:22Z

Closes Add tests for time conversions in tools package #2340
I am familiar with the contributing guidelines
Tests added
Updates entries in docs/sphinx/source/reference for API changes.
Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for 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`).
New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
Pull request is nearly complete and ready for detailed review.
Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

markcampanelli · 2024-12-23T17:21:57Z

I realize no one planned/asked for this, but I think it's a good addition and ready to be reviewed by a maintainer. Increases test coverage by a whopping 0.07%!

cwhanse

Thanks @markcampanelli

The tests expose a weakness in localize_to_utc: what happens if location has a timezone defined as an offset? That is permissible, and if done produces an unhelpful message:

    if zone.upper() == 'UTC':

AttributeError: 'int' object has no attribute 'upper'

cwhanse · 2024-12-23T19:51:18Z

pvlib/tests/test_tools.py

+    'input,expected',
+    [
+        (
+            {


Using a dict here allows the function call to be double-splatted (**input) and may make it easier to "see" each tested set of inputs and output. It's not the style used for other tests are parameterized.

I'm interested in feedback from others: is this parameterization style easier or more difficult, as a reviewer?

My 2 cents: a more compact representation of the changing values is helpful for reviewing, and is worth a bit of processing inside the test function itself. For example, it seems like the following:

( { "time": pd.DatetimeIndex( ["1974-06-22T18:30:15"], tz=ZoneInfo("Etc/GMT+5"), ), "location": location.Location( 43.19262774396091, -77.58782907414867, tz="Etc/GMT+5" ) }, pd.DatetimeIndex(["1974-06-22T23:30:15"], tz=ZoneInfo("UTC")), ), ( { "time": pd.DatetimeIndex(["1974-06-22T18:30:15"]), "location": location.Location( 43.19262774396091, -77.58782907414867, tz="Etc/GMT+5" ) }, pd.DatetimeIndex(["1974-06-22T23:30:15"], tz=ZoneInfo("UTC")), ),

could be replaced with something like this:

("1974-06-22T18:30:15", "Etc/GMT+5", 43, -77, "Etc/GMT+5", "1974-06-22T23:30:15+00:00"), ("1974-06-22T18:30:15", None, 43, -77, "Etc/GMT+5", "1974-06-22T23:30:15+00:00"),

And the test function could then contain the necessary calls to Location and pd.DatetimeIndex to construct the parameters themselves.

I think the latter format makes it easier to see what changes between each set of parameters. I found my eyes having to flick back and forth and subsequently getting lost with the former format.

markcampanelli · 2024-12-23T20:24:49Z

Thanks @markcampanelli

The tests expose a weakness in localize_to_utc: what happens if location has a timezone defined as an offset? That is permissible, and if done produces an unhelpful message:
    if zone.upper() == 'UTC':

AttributeError: 'int' object has no attribute 'upper'

Good catch! Lemme see what I might do about fixing that here (if it's not too much scope creep).

BTW: I'm pretty sure this is the sort of issue that I solved (hopefully!) by simplifying the timezone representation in pvlib.location.Location in #2343

markcampanelli · 2024-12-24T02:45:41Z

@cwhanse @kandersolar I addressed the bug Cliff spotted, and updated a Location test for that.

Due to the high repetition, I decided not to use test parameterization at all for the new tools tests. Same coverage.

Should be ready for another look.

pvlib/location.py

cwhanse · 2024-12-24T14:17:20Z

pvlib/tools.py


    Parameters
    ----------
    time : datetime.datetime, pandas.DatetimeIndex,
           or pandas.Series/DataFrame with a DatetimeIndex.
-    location : pvlib.Location object
+    location : pvlib.Location object (unused if time is localized)


Suggested change

location : pvlib.Location object (unused if time is localized)

location : pvlib.Location object (unused if ``time`` is localized)

cwhanse · 2024-12-24T14:23:48Z

pvlib/location.py

        elif isinstance(tz, (int, float)):
-            self.tz = tz
-            self.pytz = pytz.FixedOffset(tz*60)
+            self.tz = f"Etc/GMT{int(-tz):+d}"


I'm inclined to ask to revert this change. The docstring says tz can be a float. There are places that have a non-integer offset from UTC, e.g., the Marquesas are UTC−09:30 UTC−09:30. localize_to_utc is changed in this PR to examine Location.pytz rather than Location.tz, so the error we discussed won't occur.

There is a point in the Location docstring that says this:

Location objects have two timezone attributes:

* ``tz`` is a IANA timezone string. * ``pytz`` is a pytz timezone object.

While I appreciate my changes may break some things, I think this existing docstring suggests a much more sane approach. (Indeed, in the sequel using zoneinfo, I propose using one, and only one, internal representation of the timezone, instead of two, potentially with helper functions s to update this internal representation using int/float offsets, etc..)

I'm not saying revert from storing a string, but revert the int portion. If tz is truncated to an integer, how are timezones such as UTC+05:30 represented?

I'm open to doing away with accepting int or float for tz.

@cwhanse Sorry I misunderstood your meaning. I was aware that some locations are on half-hour offsets and such, but what is unclear to me is if/how that is represented in pytz (or zoneinfo for that matter). I do not see any fractional offsets in zoneinfo.available_timezones() and this also does not work for me: pytz.timezone("Etc/GMT+5.5").

I must be missing something here. Does something non-standard have to be done to support fractional offsets?

I'm open to doing away with accepting int or float for tz.

Hi again @cwhanse. It looks like the standard "non-integral-hour" offsets are only supported in the IANA standard through place names such as "Asia/Kathmandu" and "Asia/Yangon", but not through Etc/GMT+X.Y style offsets.

Given that, I think the most trouble-free thing to do is to remove the int and float options and be sure to discuss this issue in the documentation tutorial. (Also, the only supported tz strings would be the IANA ones.) Sounds like you are amenable to this.

Keeping both fields tz (only as a string) and pytz (as a pytz timezone type), then I think I should add setters that change one when the other is changed after the Location object has already been already initialized.

I think the most trouble-free thing to do is to remove the int and float options

I'm in favor of this change. @pvlib/pvlib-maintainer @pvlib/pvlib-triage your opinions?

No opinion.

TMY3 files represent timezone in numeric form, so this change may complicate some workflows. Outside of that, I never use this attribute (and don't know why I would), so I have no opinion either.

Remember that we usually have a deprecation period before outright removal.

I did some more investigating on this yesterday, and I think I see an implementation that would better support the existing int and float offsets. However, this also raised some new questions about the existing implementation. I will try to push some concrete code here by end of day for people to consider, along with the new questions raised.

Ok. I have added support back for int and non-fractional float being passed to tz in the Location constructor. I also test that pytz and zoneinfo timezones are likewise supported for initialization, and I beefed up the tests even more.

Note that the pytz Location attribute is now the single source of truth for the time zone in a Location object (which I recommend updating to a standard-library timezone.TimeZone object in #2342). self.tz simply returns the stringified self.pytz, but one can still directly set self.tz with any tz value supported by the Location intializer. Also, the tz and pytz attributes are kept in sync now, as changing one updates the other.

Ready for another review.

markcampanelli · 2025-01-09T18:37:36Z

The two test failures appear to be unrelated to my changes.

cwhanse

Do you think we should switch and regard tz as the source of truth, and pytz as the optional attribute? With an eye toward replacing pytz with python's timezone.

cwhanse · 2025-01-09T18:43:31Z

pvlib/location.py

+        from the pytz and zoneinfo packages), default 'UTC'.
+        See http://en.wikipedia.org/wiki/List_of_tz_database_time_zones for a
+        list of valid name strings for IANA time zones.
+        ints and floats must be non-fractional N-hour offsets from UTC, which


Suggested change

ints and floats must be non-fractional N-hour offsets from UTC, which

ints and floats must be whole number hour offsets from UTC, which

cwhanse · 2025-01-09T18:43:58Z

pvlib/location.py

+                    "floating point tz does not have zero fractional part: "
+                    f"{tz_}"


Suggested change

"floating point tz does not have zero fractional part: "

f"{tz_}"

"floating point tz has non-zero fractional part: "

f"{tz_}. Only whole number offsets are supported."

cwhanse · 2025-01-09T18:44:23Z

pvlib/location.py

+        else:
+            raise TypeError(
+                f"invalid tz specification: {tz_}, must be an IANA time zone "
+                "string, a non-fractional int/float UTC offset, or a "


Suggested change

"string, a non-fractional int/float UTC offset, or a "

"string, a whole number int/float UTC offset, or a "

markcampanelli · 2025-01-09T19:03:12Z

Do you think we should switch and regard tz as the source of truth, and pytz as the optional attribute? With an eye toward replacing pytz with python's timezone.

@cwhanse Ok, so we make the single source of truth the standardized IANA-supported string in the tz attribute, and make a pytz getter that converts tz to the pytz time zone using return pytz.timezone(self.tz)), and the setter that stringifies the provided pytz timezone into the tz string. I like this idea.

Question: Would it be too disruptive at this point to make the (newly implemented) pytz attribute "read only"? That is, remove the setter mentioned above, so that one can only change the time zone on an existing Location object by setting the tz attribute (with same interface as the Location initializer). I would much prefer this simplification too.

cwhanse · 2025-01-10T15:44:11Z

make the (newly implemented) pytz attribute "read only"?

I don't think we do this. Since pytz is in maintenance, it seems reasonable to expect to replace pytz with timezone (as you anticipate). At that time, the pytz attribute can be removed. I don't see the value in having both the timezone object and a string as attributes, since one can print the object and get the string representation.

markcampanelli · 2025-01-10T15:50:55Z

I decided to store the single source of truth about a Location's time zone as a psuedo-private zoneinfo.ZoneInfo object. However, the time zone is still updated (only) using the tz attribute, and so that interface is unchanged (and now very consistent for new vs. existing Location objects). However, I did make the pytz attribute read only, which avoids the synchronization issue with the tz attribute. I also added a read-only zoneinfo attribute that newly allows uses to get the time zone as the standard library's zoneinfo.ZoneInfo object (which pandas is apparently moving to in 3.0). I updated the Location's docstring accordingly. This new foundation paves the ways for #2342, but with minimal current disruption.

I also made all of @cwhanse's recommended changes.

If this implementation and the removal of directly setting the pytz attribute is ok by everyone, then I would like to add a deprecation message to the pytz attribute that says zoneinfo will be the official time-zone package going forward. I also still need to update the whatsnew and verify that the new documentation renders properly.

cwhanse

@markcampanelli let's keep this PR to the scope described in the title. Importing and adding the zoneinfo attribute feels like half of the journey to shifting from pytz to zoneinfo and I think that should be done in its own PR.

Same for fixing asv - do that in a separate PR.

cwhanse · 2025-01-10T16:27:09Z

docs/sphinx/source/whatsnew/v0.11.3.rst

@@ -26,5 +35,4 @@ Requirements

 Contributors
 ~~~~~~~~~~~~
-
-
+* Mark Campanellli (:ghuser:`markcampanelli`)


Suggested change

* Mark Campanellli (:ghuser:`markcampanelli`)

* Mark Campanelli (:ghuser:`markcampanelli`)

markcampanelli · 2025-01-10T16:32:26Z

@pvlib/pvlib-maintainer @pvlib/pvlib-triage Can someone review my asv CI fix here?

If this looks ok, then should I move the fix to a separate PR for better visibility?

markcampanelli · 2025-01-10T18:01:23Z

@pvlib/pvlib-maintainer @pvlib/pvlib-triage Can someone review my asv CI fix here?

If this looks ok, then should I move the fix to a separate PR for better visibility?

Moved to #2352, which now should go in before this.

markcampanelli added 10 commits December 22, 2024 18:52

Add tests for tools.localize_to_utc

9347f12

Add tests for datetime_to_djd and djd_to_datetime

0638773

Update what's new

c1df9a7

Appease the linter

77c0f81

Fix pandas equality tests for Python 3.9

6704d06

Fix pandas equality tests for Python 3.9 more

dbb1805

Fix pandas equality tests for Python 3.9 more more

6750709

Bump miniimum pandas to fix bad test failure

1144106

Try alternative pandas test fix

14715ed

Revert change in minimum pandas version

545c196

markcampanelli changed the title ~~[DRAFT] Add tests for time conversions in tools package~~ Add tests for time conversions in tools package Dec 23, 2024

Fix test

271fd97

kandersolar added this to the v0.11.3 milestone Dec 23, 2024

kandersolar added the testing label Dec 23, 2024

cwhanse reviewed Dec 23, 2024

View reviewed changes

markcampanelli added 4 commits December 23, 2024 14:40

Type Location's tz and pytz attributes as advertised

01263c2

Add timezone type checks to Location init test

60a5d94

Don't parameterize repetitive tests

9ab2ecf

Update whatsnew for Location bugfix

ddef8d1

markcampanelli added 2 commits December 23, 2024 19:49

Update docstring

4f17f49

Improve whatsnew formatting

a3c3e03

cwhanse reviewed Dec 24, 2024

View reviewed changes

pvlib/location.py Show resolved Hide resolved

cwhanse reviewed Dec 24, 2024

View reviewed changes

markcampanelli added 2 commits January 9, 2025 11:22

Support non-fractional int and float and pytz and zoneinfo time zones

5f59417

Appease the linter

c84801f

cwhanse reviewed Jan 9, 2025

View reviewed changes

Use zoneinfo as single source of truth and tz as interface point

195efbc

Add zoneinfo asserts in tests

1a5efed

markcampanelli added 2 commits January 10, 2025 09:19

Try to fix asv ci

e5af9ae

See if newer asv works with newer conda

67e9844

cwhanse reviewed Jan 10, 2025

View reviewed changes

markcampanelli added 6 commits January 10, 2025 09:33

Remove comments no longer needed

e35eb42

Remove addition of zoneinfo attribute

a1a0261

Revise whatsnew bugfix

8373ac4

Revise whatsnew bugfix more

eee6f51

Spell my name correctly

9662c1f

The linter strikes back again

32284ba

markcampanelli mentioned this pull request Jan 10, 2025

Replace pytz with standard library's zoneinfo #2342

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for time conversions in tools package #2341

Add tests for time conversions in tools package #2341

markcampanelli commented Dec 23, 2024 •

edited by kandersolar

Loading

markcampanelli commented Dec 23, 2024

cwhanse left a comment

cwhanse Dec 23, 2024 •

edited

Loading

kandersolar Dec 23, 2024

markcampanelli commented Dec 23, 2024 •

edited

Loading

markcampanelli commented Dec 24, 2024

cwhanse Dec 24, 2024

cwhanse Dec 24, 2024

markcampanelli Dec 24, 2024

cwhanse Dec 24, 2024

markcampanelli Dec 24, 2024

markcampanelli Dec 25, 2024

cwhanse Jan 6, 2025

adriesse Jan 7, 2025

kandersolar Jan 7, 2025

markcampanelli Jan 7, 2025

markcampanelli Jan 9, 2025

markcampanelli commented Jan 9, 2025

cwhanse left a comment

cwhanse Jan 9, 2025

cwhanse Jan 9, 2025

cwhanse Jan 9, 2025

markcampanelli commented Jan 9, 2025

cwhanse commented Jan 10, 2025

markcampanelli commented Jan 10, 2025

cwhanse left a comment •

edited

Loading

cwhanse Jan 10, 2025

markcampanelli commented Jan 10, 2025

markcampanelli commented Jan 10, 2025

	location : pvlib.Location object (unused if time is localized)
	location : pvlib.Location object (unused if ``time`` is localized)

	ints and floats must be non-fractional N-hour offsets from UTC, which
	ints and floats must be whole number hour offsets from UTC, which

		"floating point tz does not have zero fractional part: "
		f"{tz_}"

	"string, a non-fractional int/float UTC offset, or a "
	"string, a whole number int/float UTC offset, or a "

	* Mark Campanellli (:ghuser:`markcampanelli`)
	* Mark Campanelli (:ghuser:`markcampanelli`)

Add tests for time conversions in tools package #2341

Are you sure you want to change the base?

Add tests for time conversions in tools package #2341

Conversation

markcampanelli commented Dec 23, 2024 • edited by kandersolar Loading

markcampanelli commented Dec 23, 2024

cwhanse left a comment

Choose a reason for hiding this comment

cwhanse Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Dec 23, 2024 • edited Loading

markcampanelli commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Jan 9, 2025

cwhanse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Jan 9, 2025

cwhanse commented Jan 10, 2025

markcampanelli commented Jan 10, 2025

cwhanse left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markcampanelli commented Jan 10, 2025

markcampanelli commented Jan 10, 2025

markcampanelli commented Dec 23, 2024 •

edited by kandersolar

Loading

cwhanse Dec 23, 2024 •

edited

Loading

markcampanelli commented Dec 23, 2024 •

edited

Loading

cwhanse left a comment •

edited

Loading