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

SunCalc provides better results #19

Open
jfalcone opened this issue Dec 31, 2020 · 7 comments
Open

SunCalc provides better results #19

jfalcone opened this issue Dec 31, 2020 · 7 comments

Comments

@jfalcone
Copy link

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
Copy link
Owner

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
Copy link

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
Copy link

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

In:

const utcMidnight = Date.UTC(date.getFullYear(), date.getMonth(), date.getDate());

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
Copy link
Author

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
Copy link

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
Copy link

sdrsdr commented May 18, 2021

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
Copy link

sdrsdr commented May 19, 2021

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
}

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

No branches or pull requests

4 participants