Fix sun_rise_set_transit_spa returning wrong-day results (#2238)#2820
Open
Leonard013 wants to merge 1 commit into
Open
Fix sun_rise_set_transit_spa returning wrong-day results (#2238)#2820Leonard013 wants to merge 1 commit into
Leonard013 wants to merge 1 commit into
Conversation
sun_rise_set_transit_spa normalized timestamps to midnight *after* converting to UTC (introduced in v0.11.1 by pvlib#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 pvlib#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>
Hey @Leonard013! 🎉Thanks for opening your first pull request! We appreciate your If AI is used for any portion of this PR, you must vet the content |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sun_rise_set_transit_spabehavior changed in v0.11.1 #2238Why
sun_rise_set_transit_sparegressed in v0.11.1: for an evening local timestampit returns the next day's sunrise/sunset/transit. Confirmed by @cwhanse as an
unintended regression.
Root cause (
pvlib/solarposition.py):19:00 Etc/GMT+5==2000-01-02 00:00 UTC, so.normalize()in UTC pins the "dayof interest" to Jan 2. v0.11.0 pinned it to the local date.
What
Take the calendar date in the input's own timezone and re-label that midnight as UTC
(the SPA routine requires 00:00 UTC input). Strip the timezone label before
normalize()so the truncation runs on a naive index:Ordering matters.
normalize()on a tz-aware index materializes local midnight,which does not exist (spring-forward) or is ambiguous (fall-back) on DST-transition
days and raises
NonExistentTimeError/AmbiguousTimeError(e.g. America/Santiago2019-09-08, America/Havana 2020-11-01, Asia/Beirut 2021-03-28). Stripping the tz first
never constructs local midnight, so it cannot raise. The re-label to
'UTC'(nottz_convert) is required by the SPA 00:00-UTC constraint.Verified byte-for-byte identical to the pre-#2055 v0.11.0 expression
pd.DatetimeIndex(times.date).tz_localize('UTC')across 87,677 timestamps over 10zones spanning three years of DST transitions: 0 mismatches, always 00:00 UTC, 0
raises. So for every valid input this restores v0.11.0 semantics exactly and, like
v0.11.0, returns a value on those DST days (no new failure mode) while fixing
v0.11.1's wrong-day regression. Added the inline comment @cwhanse asked for.
Reconciliation with PR #2055 (why this does NOT reintroduce its problem)
PR #2055 (fixing #2054) was about clearsky day-of-year functions
(
lookup_linke_turbidity,get_extra_radiation) wrongly treating tz-aware inputs asnaive — not touched here. As a side change #2055 also swapped
sun_rise_set_transit_spafrom "midnight UTC of the local date" to "midnight UTC ofthe UTC date". Two facts make the goals reconcilable:
spa.transit_sunrise_sunsetraises
ValueError('Input dates must be at 00:00 UTC')unlessunixtime % 86400 == 0. So "normalize local thentz_convert('UTC')" (→ 05:00UTC) is not even a valid option; the function must feed a whole-day UTC value. The
only decision is which calendar date.
ambiguity concern is satisfied by the pre-existing
raise ValueError('times must be localized')guard (present in v0.11.0; Perform dayofyear-based calculations according to UTC, not local time #2055 did not add it). Given an explicittz, the local calendar day is unambiguous and matches user intent (reporter +
@cwhanse).
So restoring local-day semantics does not reintroduce any ambiguity #2055 fixed.
Tests
New
test_sun_rise_set_transit_spa_local_daycovers three cases:Etc/GMT+5, evening): sunrise/sunset/transit land on thelocal day 2000-01-01, same-local-day rows identical, pinned v0.11.0 values (to the
second). FAILS before the fix (row 2 → 2000-01-02).
Etc/GMT-9, early-morning = previous UTC day): mirrorimage; local day 2000-07-01, identical rows.
America/Santiago, evening on the 2019-09-08 spring-forwardday): asserts local day 2019-09-08 and that the call does not raise. This
guards a bug an earlier iteration of this fix had (it raised
NonExistentTimeErrorunder the
.normalize().tz_localize(None)order) — flagged transparently.Full
tests/test_solarposition.py: 60 passed, 21 skipped. No existing test neededchanging. flake8 clean on all changed lines.
Reviewer notes
interest as the input's local calendar date (v0.11.0 behavior,
@cwhanse-confirmed).
times.tz_localize(None).normalize().tz_localize('UTC')expression isorder-sensitive (strip tz before normalize → DST-safe) and re-labels rather than
tz_converts (SPA 00:00-UTC constraint). The inline comment flags both so neitheris "simplified" away.
Prepared with AI assistance (Claude Code): the reproduction, the #2055 analysis, the
fix, and the tests were AI-drafted and independently reviewed — that review caught and
regression-guarded a midnight-DST crash in an earlier version of the fix, as noted
above. The reasoning and verification are documented in full.