Skip to content
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

Reconfigure offset calculation to fix UTC rollover. Fixes: #81, #82, and #83, #90

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

shanedoolane
Copy link

In my tests validated against real data, this fixes sunset/sunrise issues for the location i'm working with detailed in #83.

offset comparison no longer causes previous day to be returned when local time rolled over UTC date

Test:

start_date = datetime.date(2023, 1, 1)
latitude = 37.4
longitude = -122.1
observer = astral.sun.Observer(latitude, longitude)

def daylight_hours(current_date):
    sunrise = astral.sun.sunrise(observer, date=current_date)  # returns UTC
    sunset = astral.sun.sunset(observer, date=current_date) # returns UTC

    daylight_hours = (sunset - sunrise).total_seconds() / 3600
    return daylight_hours

Old:
Screenshot 2023-05-06 at 23 16 43

New:
image

offset comparison no longer causes previous day to be returned when local time rolled over UTC date
the offset is actually needed to shift the date by 1. If you don't subtract the delta in the check function on line 882, the value error is thrown.
@shanedoolane
Copy link
Author

add 5202da6 to apply offset and block value error from being thrown if the date doesn't match by applying delta term. appears to work for PST:

checking date: 2023-06-30 ====================================================
====>sunrise: 2023-06-30 12:XXX+00:00
====>sunset: 2023-07-01 03:XXX+00:00
daylight hours: 14.XXX
checking date: 2023-07-01 ====================================================
====>sunrise: 2023-07-01 12:XXX+00:00
====>sunset: 2023-07-02 03:XXX+00:00
daylight hours: 14.XXX

Screenshot 2023-05-07 at 00 39 05

Still have an issue with singularity when the day length is exactly 12 and sunset is at midnight
@shanedoolane
Copy link
Author

Ok maybe not as simple of a fix as I thought. Went to test some more locations and miraculously found an edge case that fails the UTC rollover conditions. I randomly picked coords point but somehow stars aligned:

Observer:

# Set location
latitude = 41.908
longitude = -87.655

# Create observer object
observer = astral.sun.Observer(latitude, longitude)

As we can see on 3/18 local date in chicago, sunset is basically at midnight UTC on 3/18. I think because of a floating operation, the offset condition is applied and it bumps the date back to 3/17 UTC causing a singularity. Not sure how best to fix this.

date:  2023-03-15 , sunrise: 2023-03-15 12:02:58.922185+00:00  sunset: 2023-03-15 23:56:44.442507+00:00  daylight hrs:  11.895977867222221
date:  2023-03-16 , sunrise: 2023-03-16 12:01:16.919834+00:00  sunset: 2023-03-16 23:57:52.582803+00:00  daylight hrs:  11.94323971361111
date:  2023-03-17 , sunrise: 2023-03-17 11:59:34.692298+00:00  sunset: 2023-03-17 23:59:00.542142+00:00  daylight hrs:  11.990513845555554
date:  2023-03-18 , sunrise: 2023-03-18 11:57:52.270697+00:00  sunset: 2023-03-17 23:59:00.542142+00:00  daylight hrs:  -11.981035709722223
date:  2023-03-19 , sunrise: 2023-03-19 11:56:09.685995+00:00  sunset: 2023-03-20 00:00:08.330443+00:00  daylight hrs:  12.066290124444444
date:  2023-03-20 , sunrise: 2023-03-20 11:54:26.969014+00:00  sunset: 2023-03-21 00:02:23.381227+00:00  daylight hrs:  12.132336725833333
date:  2023-03-21 , sunrise: 2023-03-21 11:52:44.150441+00:00  sunset: 2023-03-22 00:03:30.716642+00:00  daylight hrs:  12.179601722500001
date:  2023-03-22 , sunrise: 2023-03-22 11:51:01.260846+00:00  sunset: 2023-03-23 00:04:37.920994+00:00  daylight hrs:  12.226850041111112

Screenshot 2023-05-07 at 01 42 53

@jlopezarriaza
Copy link

@shanedoolane and @sffjunkie, I'm curious if there's any movement on this or if there are plans on merging this to master?

Happy to try to help wherever I can to solve this edge case.

@shanedoolane
Copy link
Author

@jlopezarriaza haven't gotten around to fixing yet. Want to pull the branch and look at it?

@jlopezarriaza
Copy link

jlopezarriaza commented Aug 29, 2023

I'm happy to check it out. I found a workaround by doing things in local time zone rather than UTC, but that doesn't seem optimal.

@Voutsi
Copy link

Voutsi commented Sep 1, 2023

Hi @shanedoolane
thanks a lot for the effort, I am also very interested on this fix.
I tried the PR branch with the following code snippet

from datetime import datetime
import astral
from astral.sun import sun

observer = astral.Observer(latitude=-24.6272, longitude=-70.4039, elevation=2200)
timestamp =  datetime(2023, 9, 23)
helios = sun(observer, timestamp, dawn_dusk_depression=18)
dusk = helios['dusk']
dawn = helios['dawn']
print ("dawn", dawn, "dusk", dusk)

a first comment is that a small fix is needed in the functions calculating dusk and dawn in order to run this test code, i.e.
if tot_date != date -> if tot_date != date - delta

but apart of that I see some confusing behaviour: If I test the code with the fix branch I get this output (adding some printout):

time of transit date 2023-09-25 requested date 2023-09-23
dawn 2023-09-23 09:07:47.206584+00:00 dusk 2023-09-24 00:00:19.982675+00:00

while when I run with the master branch from the main repo I get this:

dawn 2023-09-23 09:07:47.206584+00:00 dusk 2023-09-23 00:00:19.982675+00:00

So there are 3 things that are puzzling me here

  • the Time of Transit date is 2 days after the requested date
  • the PR branch gives me the dusk for the next day (24th of September) that the one requested
  • I am getting identical timestamp for the dusk from the PR branch and the main branch, however the main branch gives me the timestamp for 23 of September while the PR branch from 24. These 2 can't be the same.

@shanedoolane
Copy link
Author

@Voutsi @jlopezarriaza let me take a look over the weekend for a fix

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.

3 participants