Skip to content

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

Open
Leonard013 wants to merge 1 commit into
pvlib:mainfrom
Leonard013:fix/2238-sun-rise-set-local-day
Open

Fix sun_rise_set_transit_spa returning wrong-day results (#2238)#2820
Leonard013 wants to merge 1 commit into
pvlib:mainfrom
Leonard013:fix/2238-sun-rise-set-local-day

Conversation

@Leonard013

Copy link
Copy Markdown
  • Closes sun_rise_set_transit_spa behavior changed in v0.11.1 #2238
  • I am familiar with the contributing guidelines
  • I attest that all AI-generated material has been vetted for accuracy and is in compliance with the pvlib license
  • Tests added
  • Adds description and name entries in the appropriate "what's new" file
  • New code is fully documented (inline comment added, as requested in the issue)

Why

sun_rise_set_transit_spa regressed in v0.11.1: for an evening local timestamp
it returns the next day's sunrise/sunset/transit. Confirmed by @cwhanse as an
unintended regression.

import pandas as pd, pvlib
times = pd.to_datetime(['2000-01-01 18:00', '2000-01-01 19:00']).tz_localize('Etc/GMT+5')
pvlib.solarposition.sun_rise_set_transit_spa(times, 40, -80)['sunrise']
# current main:
#   2000-01-01 18:00 -> 2000-01-01 07:41:...   (correct day)
#   2000-01-01 19:00 -> 2000-01-02 07:41:...   (WRONG: next day)
# Both rows are the same LOCAL calendar day and should give the same sunrise.

Root cause (pvlib/solarposition.py):

times_utc = times.tz_convert('UTC')
unixtime = _datetime_to_unixtime(times_utc.normalize())   # normalize AFTER tz_convert

19:00 Etc/GMT+5 == 2000-01-02 00:00 UTC, so .normalize() in UTC pins the "day
of 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:

unixtime = _datetime_to_unixtime(
    times.tz_localize(None).normalize().tz_localize('UTC'))

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/Santiago
2019-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' (not
tz_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 10
zones 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 as
naive — not touched here. As a side change #2055 also swapped
sun_rise_set_transit_spa from "midnight UTC of the local date" to "midnight UTC of
the UTC date". Two facts make the goals reconcilable:

  1. The SPA kernel hard-requires 00:00 UTC inputspa.transit_sunrise_sunset
    raises ValueError('Input dates must be at 00:00 UTC') unless
    unixtime % 86400 == 0. So "normalize local then tz_convert('UTC')" (→ 05:00
    UTC) is not even a valid option; the function must feed a whole-day UTC value. The
    only decision is which calendar date.
  2. For sunrise/sunset the date is inherently a local-day concept. Perform dayofyear-based calculations according to UTC, not local time #2055's
    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 explicit
    tz, 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_day covers three cases:

  • Issue's exact case (Etc/GMT+5, evening): sunrise/sunset/transit land on the
    local 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).
  • Positive-offset zone (Etc/GMT-9, early-morning = previous UTC day): mirror
    image; local day 2000-07-01, identical rows.
  • Midnight-DST zone (America/Santiago, evening on the 2019-09-08 spring-forward
    day): 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 NonExistentTimeError
    under the .normalize().tz_localize(None) order) — flagged transparently.

Full tests/test_solarposition.py: 60 passed, 21 skipped. No existing test needed
changing. flake8 clean on all changed lines.

Reviewer notes

  • Day-of-interest semantics is the key design point: this treats the day of
    interest as the input's local calendar date (v0.11.0 behavior,
    @cwhanse-confirmed).
  • The times.tz_localize(None).normalize().tz_localize('UTC') expression is
    order-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 neither
    is "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.

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>
@github-actions

Copy link
Copy Markdown

Hey @Leonard013! 🎉

Thanks for opening your first pull request! We appreciate your
contribution. Please ensure you have reviewed and understood the
contributing guidelines.

If AI is used for any portion of this PR, you must vet the content
for technical accuracy.

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

Successfully merging this pull request may close these issues.

sun_rise_set_transit_spa behavior changed in v0.11.1

1 participant