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

Different results are given for the same solar event depending on the date-time given as input #30

Closed
johanthoren opened this issue Aug 5, 2021 · 3 comments

Comments

@johanthoren
Copy link

Hi, and thank you for providing this library. I'm using it to provide solar (and lunar) calculations in a Hebrew calendar library for Clojure. As I'm not a Java developer I apologize for only being able to provide feedback from a Clojure perspective.

I have a function named calculate-sun-events:

(defn- calculate-sun-events
  [lat lon date]
  (let [tz (tz? date)]
    (-> (SunTimes/compute)
        (.on date)
        (.at lat lon)
        (.oneDay)
        (.timezone (str tz))
        (.execute))))

Depending on what date I provide, I get slightly different results for the same solar events:

(calculate-sun-events 58 12 (go-back (t/hours 6) (now)))
;; => #object[org.shredzone.commons.suncalc.SunTimes 0x5884dce1 "SunTimes[rise=2021-08-06T05:16:12.951668+02:00[Europe/Stockholm], set=2021-08-05T21:20:50.951668+02:00[Europe/Stockholm], noon=2021-08-06T13:17:40.951668+02:00[Europe/Stockholm], nadir=2021-08-06T01:18:17.951668+02:00[Europe/Stockholm], alwaysUp=false, alwaysDown=false]"]
(calculate-sun-events 58 12 (go-back (t/hours 10) (now)))
;; => #object[org.shredzone.commons.suncalc.SunTimes 0x24c96f8 "SunTimes[rise=2021-08-06T05:16:13.347419+02:00[Europe/Stockholm], set=2021-08-05T21:20:51.347419+02:00[Europe/Stockholm], noon=2021-08-05T13:17:48.347419+02:00[Europe/Stockholm], nadir=2021-08-06T01:18:18.347419+02:00[Europe/Stockholm], alwaysUp=false, alwaysDown=false]"]

lat is 58, and lon is 12 in both examples above. date is 6 hours ago and 10 hours ago since the current system time as a ZonedDateTime object. Looking at the events I would expect them to be exactly the same in both examples since they occur during the same day, but there are slight differences.

Could this be a bug?

@shred
Copy link
Owner

shred commented Aug 5, 2021

Thank you for your report. Yes, one could expect that the result is exactly the same because it is the same event.

However, astronomical calculations are quite complex. This library uses approximation algorithms for calculation, to keep the CPU load low. Depending on the preconditions (e.g. starting time of the time window), the results will then be slightly different due to rounding errors.

Please also see the accuracy section in the ReadMe file. This library was never designed to be precise down to a second, but only to about a minute. This is given in your example, so no, it's not a bug. 🙂

You can use LocalDateTime.truncatedTo() to truncate the result to full minutes. It should give consistent results.

As a side node: To get meaningful sun times in a precision of a second or less, one would also have to consider factors like air pressure, the current temperatures in different atmospheric layers, topological obstacles (like mountains or large buildings) etc.

@johanthoren
Copy link
Author

Thank you for your prompt response. I think I misread it as "the time calculated for an event will always be the same, but it's true accuracy will be within minutes rather than seconds". I'll adjust my code accordingly.

Thank you for the clarification. You may close this issue as far as I am concerned.

@shred
Copy link
Owner

shred commented Aug 5, 2021

You're welcome! I'm glad I could clarify it.

@shred shred closed this as completed Aug 5, 2021
johanthoren added a commit to johanthoren/luminary that referenced this issue Aug 6, 2021
After feedback in the issue
shred/commons-suncalc#30 I have decided to
stop calculating all sun events twice (to work around differing results
depending on the input date) and instead truncate the results to minutes
instead of seconds.

This should give some performance improvement, and will also better
reflect the actual precision of the calculations.

I've added some additional tests to verify that the same minute is
the result no matter when during the day the input date is. Some
additional tests could be added in the future.
johanthoren added a commit to johanthoren/luminary that referenced this issue Aug 16, 2021
After feedback in the issue
shred/commons-suncalc#30 I have decided to
stop calculating all sun events twice (to work around differing results
depending on the input date) and instead truncate the results to minutes
instead of seconds.

This should give some performance improvement, and will also better
reflect the actual precision of the calculations.

I've added some additional tests to verify that the same minute is
the result no matter when during the day the input date is. Some
additional tests could be added in the future.
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

2 participants