search

udivankin / sunrise-sunset

Sunrise and sunset time calculation for given coordinates. ~1kb minified with zero dependencies.
71 stars 17 forks source link

SunCalc provides better results #19

Open jfalcone opened 3 years ago

jfalcone commented 3 years ago

I know this problem has been posted before and I know you based the code on Kevin Boone's code, but his Solunar code returns for Los Angeles:

Dec 31 2020, Sunrise: 06:59, Sunset: 16:55

SunCalc returns:

sunrise: 2020-12-31T15:25:27.510Z sunset: 2021-01-01T01:02:54.632Z (note that in Zulu, today's sunset is on 01-01 for my Pacific time zone)

sunrise-sunset returns:

sunrise: 2020-12-31T15:24:21.201Z sunset: 2020-12-31T01:02:22.328Z (sunset is in fact yesterday's sunset, not the sunset for 12-31 which of course is 01-01 in Zulu)

So with all due respect, when I ask for sunrise and sunset from Now, I expect these to be the values for today, not a combination of today and yesterday. You have the geocoordinates, so it is possible to ascertain exactly when the sunrise and sunset are for today in that geolocation.

So I am using the 4-year-old SunCalc code because it provides what I expect, not some values based on algorithms which don't appear to have been translated correctly from Mr. Boone's code.

udivankin commented 3 years ago

Thank you @jfalcone for your opinion, tbh I didn't even think it could be a problem since sunrise/sunset times are pretty rough estimates in real life (considering the terrain, weather conditions, etc), but some edge cases definitely need to be covered and documented. I Will look into it once I have enough time for it!

billbliss commented 3 years ago

OMG this one just bit me too. I'm also in the Pacific timezone and it never occurred to me that sunset would return the sunset for the previous day.

billbliss commented 3 years ago

I found the problem. The getSunset() function will only work for local times east of GMT.

In: https://github.com/udivankin/sunrise-sunset/blob/088f8c539714ba1460b7701d324c8791951c3728/src/index.ts#L123

Note that date is treated as local time. At midnight UTC time, locations west of GMT are always still on the day before. As written the function will always return the (presumably correct) sunset for the passed-in date, but will say it's on the day before.

It's a trivial change, I'll submit a PR. Well, trivial to implement once I figured it out. These timezone calculation issues are a pain!

jfalcone commented 3 years ago

Frankly, not worth the trouble fixing this. Given this bug, I can imagine that there are a dozen others hiding in there because the author thought that GMT was (and I guess still is) the center of the universe. Just don't trust random Github code anymore. Period. Sorry for being snarky but this obvious bug cost me a lot of my time. I recommend that the author withdraw the code until such time as he can test it appropriately.

billbliss commented 3 years ago

I took a crack at fixing it and I agree. Dealing with timezones is a pain in JavaScript, but even aside from that it's not clear that it's working correctly east of GMT.

SunCalc works differently, and it works correctly.

sdrsdr commented 3 years ago

There is a off-by-one in getDayOfYear. The correct fnction imho is 

function getDayOfYear(date: Date): number {
    return Math.ceil((date.getTime() - Date.UTC(date.getUTCFullYear(), 0, 1)) / 8.64e7)+1;
}
sdrsdr commented 3 years ago

also over the polar circle we have to check if there will be sunrise/sunset this given day:

if (cosLocalHourAngle>1 || cosLocalHourAngle<-1) {
    return ....; //the sun will not be above or below horizon this day
}