sunrise-sunset icon indicating copy to clipboard operation
sunrise-sunset copied to clipboard

SunCalc provides better results

Open jfalcone opened this issue 3 years ago • 7 comments

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.

jfalcone avatar Dec 31 '20 15:12 jfalcone

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!

udivankin avatar Feb 20 '21 09:02 udivankin

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 avatar Mar 12 '21 16:03 billbliss

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!

billbliss avatar Mar 13 '21 00:03 billbliss

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.

jfalcone avatar Mar 13 '21 00:03 jfalcone

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.

billbliss avatar Mar 14 '21 08:03 billbliss

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 avatar May 18 '21 21:05 sdrsdr

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
}

sdrsdr avatar May 19 '21 07:05 sdrsdr