Perform dayofyear-based calculations according to UTC, not local time by yhkee0404 · Pull Request #2055 · pvlib/pvlib-python

yhkee0404 · 2024-05-19T22:32:09Z

Closes Clearsky functions handling tz-aware datetimes as tz-naive, contrary to user guide #2054
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.

A similar solution as #333

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

cwhanse · 2024-06-05T22:29:01Z

@yhkee0404 did you close this intentionally? If you aren't able to complete the PR, let us know and one of us could take it from here.

yhkee0404 · 2024-06-05T22:40:45Z

@cwhanse Oh, sorry for confusion. I was in the midst of my struggle with the problem and was going to reopen it after several hours. Thank you for your suggestions! Now I think it is better to keep it open. Any help would be welcome!

kandersolar

Thanks @yhkee0404 for this thorough PR! I have a few questions below that I hope you can help me understand.

I also think the title of this PR should be edited to reflect the broader scope of the changes.

kandersolar · 2024-09-06T13:53:08Z

+    # times must be localized
+    if not times.tz:
+        raise ValueError('times must be localized')
+
    # hours - timezone = (times - normalized_times) - (naive_times - times)
-    if times.tz is None:
-        times = times.tz_localize('utc')


I'm not sure I agree with this change. The docstring says the input timestamps "must be localized to the timezone for the longitude", which is inconsistent with the previous behavior (localize to UTC if not already localized), so I can see some change being needed here. But why is this the correct change?

It was added in accordance with sun_rise_set_transit_spa and sun_rise_set_transit_ephem as well as the docstring. I agree with the previous convention of raising a ValueError since it prevents users from unwittingly calling the functions naively without assuming any timezones and ignoring the fact that an hour angle is counted up from zero at noon in a predefined local timezone. Please refer to the followings:

pvlib-python/pvlib/solarposition.py

Lines 448 to 452 in 524fa55

# times must be localized

if times.tz:

tzinfo = times.tz

else:

raise ValueError('times must be localized')

pvlib-python/pvlib/solarposition.py

Lines 560 to 564 in 524fa55

# times must be localized

if times.tz:

tzinfo = times.tz

else:

raise ValueError('times must be localized')

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

yhkee0404 · 2024-09-06T18:50:50Z

Many thanks to @cwhanse for leading this PR into the right way, and @kandersolar for taking time for a further reconciliation from other aspects! Please feel free to fix or revert any including the title or whatsnew with your excellent ideas.

cwhanse · 2024-09-11T15:33:21Z

@yhkee0404 we should add a few tests to improve coverage. Let me know if you would like my help with that.

yhkee0404 · 2024-09-12T07:14:38Z

@cwhanse I have just made some patchs to improve coverage but I am not sure they would work. I would really appreciate if you could please help me. This is my first time to work with Codecov. Is there a way to see the difference locally on my own, or should I just wait for your manual approval for codecov workflow?

cwhanse · 2024-09-12T15:13:47Z

@yhkee0404 the additional tests are good enough. I don't see why codecov is flagging that one line. Like you I don't know how to run codecov locally, and just let the CI do it for me.

yhkee0404 · 2024-09-13T06:33:20Z

Great! It seems calling solarposition.spa_c in test_spa_c_physical and test_spa_c_physical_dst on pvlib/tests/test_solarposition.py both fail on Codecov because of lack of spa_c_files which makes these lines fail accordingly: from pvlib.spa_c_files.spa_py import spa_calc, has_spa_c and requires_spa_c. It is different from the case of other libraries like ephem and numba since both has_ephem and has_numba are True on Codecov while has_spa_c() is evaluated as False: https://app.codecov.io/gh/pvlib/pvlib-python/pull/1944/blob/pvlib/tests/conftest.py#L136 In another word, to test spa_c which is not imported, we should install it on Codecov somehow like this direction: https://pvlib-python.readthedocs.io/en/stable/user_guide/installation.html?highlight=spa_c_files#nrel-spa-algorithm

kandersolar

I believe I have finally understood this PR. It it looks correct to me. I have only a couple revisions to the what's new text to fully communicate the changes.

In another word, to test spa_c which is not imported, we should install it on Codecov somehow like this direction

Unfortunately, licensing reasons prevent us from testing the spa_c code in CI. Any coverage issues related to spa_c should be ignored :)

yhkee0404 · 2024-09-18T07:54:34Z

Thank you for your thorough review and suggestion that clarifies this PR in more datail👍!

kandersolar · 2024-09-24T17:56:21Z

Thanks again @yhkee0404 for finding, investigating, and fixing this complex issue!

…2820) sun_rise_set_transit_spa normalized timestamps to midnight *after* converting to UTC (introduced in v0.11.1 by #2055). An evening-local timestamp is the next calendar day in UTC, so its day of interest was shifted forward and the function returned the following day's sunrise, sunset, and transit. Use the input's local calendar date as the day of interest, restoring the pre-v0.11.1 behavior. The SPA routine requires 00:00 UTC input, so the local date is re-labeled (not converted) to UTC. The timezone label is stripped before normalize() so truncation runs on a naive index: normalizing a tz-aware index would build local midnight, which does not exist (spring-forward) or is ambiguous (fall-back) on DST-transition days and would raise. This yields unixtimes byte-for-byte identical to the v0.11.0 implementation for all valid inputs. Adds an inline comment explaining the timezone handling (requested in the issue) and a regression test covering negative and positive offsets and a midnight-DST zone. Fixes #2238 This change was drafted with AI assistance (Claude); all AI-generated material has been vetted for accuracy and is license-compliant. Co-authored-by: Claude <noreply@anthropic.com>

yhkee0404 added 6 commits May 20, 2024 02:03

Add clearsky.ipynb (Remove before merge)

6c4c32a

Fix clearsky.lookup_linke_turbidity to be timezone-aware

9ce02d5

Fix irradiance.get_extra_radiation to be timezone-aware

0ad7a68

Rerun and clear errors in clearsky.ipynb

05961d2

Remove clearsky.ipynb

45a36cb

Update what's new

b00a2d4

yhkee0404 marked this pull request as ready for review May 19, 2024 22:33

yhkee0404 changed the title ~~Clearsky timezone~~ Fix tz-naive linke_turbidity and dni_extra to be tz-aware May 19, 2024

yhkee0404 mentioned this pull request May 20, 2024

Clearsky functions handling tz-aware datetimes as tz-naive, contrary to user guide #2054

Closed

yhkee0404 closed this May 26, 2024

yhkee0404 deleted the clearsky-timezone branch May 26, 2024 02:53

yhkee0404 restored the clearsky-timezone branch May 26, 2024 03:17

yhkee0404 added 2 commits May 26, 2024 14:33

Merge branch 'main' into clearsky-timezone

7bb7e02

Merge branch 'main' into clearsky-timezone

6d1ab57

yhkee0404 reopened this May 26, 2024

yhkee0404 force-pushed the clearsky-timezone branch from f4d42e8 to 6d1ab57 Compare May 26, 2024 05:54

cwhanse reviewed May 27, 2024

View reviewed changes

Comment thread pvlib/clearsky.py Outdated

Comment thread pvlib/irradiance.py

yhkee0404 added 2 commits June 1, 2024 23:53

Add docstring Notes to lookup_linke_turbidity

3fed282

Add tools._pandas_to_utc

6f98df7

cwhanse reviewed Jun 4, 2024

View reviewed changes

Comment thread docs/sphinx/source/whatsnew/v0.11.0.rst Outdated

Comment thread pvlib/tests/test_irradiance.py Outdated

Comment thread pvlib/tests/test_irradiance.py Outdated

Comment thread pvlib/tools.py

Comment thread pvlib/tools.py

Comment thread pvlib/tools.py Outdated

Update what's new

fe5b5c1

Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>

yhkee0404 closed this Jun 5, 2024

Add docstring Notes to _pandas_to_doy

97afcb8

yhkee0404 added 3 commits June 6, 2024 07:49

Revert _datetimelike_scalar_to_datetimeindex

defddfd

Revert tests.test_irradiance.times

a08ca79

Replace lowercase utc with uppercase

6baa1ed

yhkee0404 reopened this Jun 5, 2024

yhkee0404 force-pushed the clearsky-timezone branch from 28a94d5 to fe5b5c1 Compare June 5, 2024 22:58

kandersolar reviewed Sep 6, 2024

View reviewed changes

Update docs/examples/irradiance-transposition/plot_seasonal_tilt.py

33f778f

Co-authored-by: Kevin Anderson <kevin.anderso@gmail.com>

yhkee0404 added 4 commits September 9, 2024 17:19

Revert benchmarks/benchmarks/solarposition.py

16f0e5a

Revert test_lookup_linke_turbidity_nointerp

2745954

Revert solrad.py

eb2707f

Revert plot_seasonal_tilt.py except notes

ab953b5

yhkee0404 added 6 commits September 12, 2024 15:50

Update test_sun_rise_set_transit_spa with delta_t

b8246c0

Update test_nrel_earthsun_distance with delta_t

7da25f4

Update test_sun_rise_set_transit_geometric with naive tz

70d06cb

Update test_hour_angle with inversion and naive tz

dea79d3

Remove _is_leap_year from _interpolate_turbidity

2143c94

Merge branch 'main' into clearsky-timezone

80a6afb

kandersolar approved these changes Sep 17, 2024

View reviewed changes

Comment thread docs/sphinx/source/whatsnew/v0.11.1.rst Outdated

Comment thread docs/sphinx/source/whatsnew/v0.11.1.rst Outdated

kandersolar changed the title ~~Fix tz-naive linke_turbidity and dni_extra to be tz-aware~~ Perform dayofyear-based calculations according to UTC, not local time Sep 17, 2024

yhkee0404 added 2 commits September 18, 2024 16:35

Merge branch 'main' into clearsky-timezone

87cd719

Update docs/sphinx/source/whatsnew/v0.11.1.rst

c4b65e1

kandersolar mentioned this pull request Sep 23, 2024

v0.11.1 release plan #2220

Closed

11 tasks

kandersolar merged commit d45790a into pvlib:main Sep 24, 2024

yhkee0404 mentioned this pull request Sep 29, 2024

Timezone mismatch in Bird Clear Sky Model NatLabRockies/SolarResourceModelsandTools#4

Open

kandersolar mentioned this pull request Oct 4, 2024

sun_rise_set_transit_spa behavior changed in v0.11.1 #2238

Closed

yhkee0404 mentioned this pull request Sep 3, 2025

Potential bug with solarposition.py #2502

Open

Leonard013 mentioned this pull request Jul 12, 2026

Fix sun_rise_set_transit_spa returning wrong-day results (#2238) #2820

Merged

6 tasks

	# times must be localized
	if times.tz:
	tzinfo = times.tz
	else:
	raise ValueError('times must be localized')

Uh oh!

Conversation

yhkee0404 commented May 19, 2024 • edited by cwhanse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwhanse commented Jun 5, 2024

Uh oh!

yhkee0404 commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kandersolar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kandersolar Sep 6, 2024

Choose a reason for hiding this comment

Uh oh!

yhkee0404 Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yhkee0404 commented Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwhanse commented Sep 11, 2024

Uh oh!

yhkee0404 commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwhanse commented Sep 12, 2024

Uh oh!

yhkee0404 commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kandersolar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yhkee0404 commented Sep 18, 2024

Uh oh!

kandersolar commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yhkee0404 commented May 19, 2024 •

edited by cwhanse

Loading

yhkee0404 commented Jun 5, 2024 •

edited

Loading

kandersolar left a comment •

edited

Loading

yhkee0404 Sep 6, 2024 •

edited

Loading

yhkee0404 commented Sep 6, 2024 •

edited

Loading

yhkee0404 commented Sep 12, 2024 •

edited

Loading

yhkee0404 commented Sep 13, 2024 •

edited

Loading