More accurate rising, transit and setting times

[rhannequin]

This change is a complete refactoring of rising and setting times and azimuth.
It is based on Astronomical Algorithms by Jean Meeus with a more complex but more accurate method than the ones used so far.
It also introduces transit time and altitude.

The overall precision of these events have been increased, especially for the Sun. Most of the times are accurate with the IMCCE within 2 minutes precision, with results sometimes accurate to a few seconds.

More work may be needed in the future to increase the precision for distant stars, that should be in theory simpler to deal with but are less accurate for some reason (angles are off by a dozen minutes of arc).

The following methods have been added to Sun:

• #rise_transit_set_times
• #rise_set_azimuths
• #transit_time
• #transit_altitude
  They all require an observer key argument. Memoization on the observer has been implemented so that each of these methods calls don't compute the values all over again.

This change includes a breaking change:
Astronoby::Body is dropped. It will probably be replaced by something like DistantStar, Moon and Planet in the future, in the same way we have Sun.

date = Date.new(2015, 2, 5)
epoch = Astronoby::Epoch.from_time(date)
observer = Astronoby::Observer.new(
  latitude: Astronoby::Angle.from_degrees(38),
  longitude: Astronoby::Angle.from_degrees(-78)
)
sun = Astronoby::Sun.new(epoch: epoch)

sun.rising_time(observer: observer)
# => 2015-02-05 12:12:59 UTC

sun.rising_azimuth(observer: observer).str(:dms)
# => "+109° 46′ 43.1427″"

sun.transit_time(observer: observer)
# => 2015-02-05 17:25:59 UTC

sun.transit_altitude(observer: observer).str(:dms)
# => "+36° 8′ 15.7669″"

sun.setting_time(observer: observer)
# => 2015-02-05 22:39:27 UTC

sun.setting_azimuth(observer: observer).str(:dms)
# => "+250° 23′ 33.6177″"

# Also available in more compact forms:
sun.rise_transit_set_times(observer: observer)
# => [..., ..., ...]

sun.rise_set_azimuths(observer: observer)
# => [..., ...]

[janfri]

* `Sun`'s rising/setting time/azimuth methods have been replaced by:
  
  * `#rise_transit_set_times` which returns an array of times
  * `#rise_set_azimuths` which returns an array of azimuth angles

What's the motivation for this change? For me it seems not to be an improvement:

1. The names of the methods are a bit long: #rise_transit_set_times instead of a simple #rise_time, #transit_time or #set_time
2. For every call it calculates three or two values even if I only interested in one of the these.

btw. astronoby is a great library with very readable code so far. :-)

[rhannequin]

@janfri Thanks a lot for the feedback. It means a lot to me to know some people are actually following the project and I'm glad to receive feedback to increase its quality 🙏

The main reason of this API change is to be more performant.
The implemented algorithm calculates almost everything at the same time: rise/transit/set times, rise/set azimuths and transit elevation. This new algorithm is fairly more expensive than the previous one, which already suffered from having to calculate almost the same data multiple times when calling rising_time, setting_time, rising_azimuth, etc, independently.

By implementing all these methods independently, the program will compute all the data at each call, regardless of if only one value will be accessed by the developer in the end.
This is mainly because the Sun is initialized with an epoch, but observation methods take an observer parameter. The epoch is memoized, but not the observer.

If would need to find a way to memoize each RiseTransitSet object for each different value of observer to provide separated methods on Sun while keeping the computation performant.

Another way to solve this would be to add observer to the Sun's constructor, but it doesn't feel right to me. The Sun, as a celestial object, has attributes based on an instant in time (equation of time, ecliptic coordinates, true anomaly, etc), and is not affected by who it is observed by.

Do you have an opinion on how to resolve this?

[rhannequin]

@janfri Your feedback (and what I first answered) got me thinking and I think you're right, the developer should not be impacted by internal constraints. If the calculation is expensive and calculates all the data immediately, the developer should not have to adapt to this situation but only benefit from a clear and pleasant API.

I implemented a memoization mechanism based on the observer, by making it behave like a value object: 7979386
This way, is it possible to call sun.rising_time(observer: observer) and sun.setting_time(observer: observer) with only one calculation.

I kept #rise_transit_set_time and #rise_set_azimuths as I noticed in other libraries that it was quite custom to provide an array or tuple for this data. I can see the value of getting all the times or azimuths in one variable assignment statement, without making the developer have to go this way.

Thank you for the feedback, I would be happy to know what you think.

[janfri]

If I understand it correct the new algorithm calculates data which is horizon related: times and azimuths. If I'm correct it could be also used to calculate values for twilight related data (astronomical twilight, nautical twilight, civil twilight). In your current approach you would need a method which returns an array with very many values and a very long name.

So maybe it would be better to return an object with explicit accessors (for example implemented as Struct):

sun = Astronoby::Sun.new(epoch: epoch)

he = sun.horizontal_events(observer: observer)

he.astronomical_twilight_end_time
he.nautical_twilight_end_time
he.civil_twilight_end_time
he.rising_time
he.transit_time
he.setting_time
he.civil_twilight_begin_time
he.nautical_twilight_begin_time
he.astronomical_twilight_begin_time

# same for azimuth

Yes, we need a better name for horizontal_events but I hope the idea is clear.

The advantages of this approach would be:

1. Only one calculation for all "events"
2. No complex memoization needed, because all results are stored in one object
3. Easy to expand for further "events": Add accessors to the class of the returned object
4. Shorter method names

The only disadvantage I see is a new class with a good name and an according method name for the sun instance. ;-)

What do you think?

[rhannequin]

@janfri Something is bothering me about having a "horizontal events" class: the fact that it looks universal, while some events only apply to the Sun.

I have this PR ready for twilight times, they are currently available from Sun. Rising, transit and setting times (and azimuths) apply to any body, like stars and planets, but the twilight only applies to the Sun's light.

I can see the value of extracting twilight events times into a dedicated class if it doesn't feel right to have them in Sun, but I feel like they still should be separated from "universal" events like rising and setting that apply to any body on the celestial sphere.
The algorithm (from Practical Astronomy with your Calculator or Spreadsheet) requires data from the observer and the sunrise and sunset times, which are publicly available from Sun, so it is technically not a problem to extract twilight methods into a dedicated class. But in that case, the sources of information for sunrise/sunset + twilight times are separated, while they are all related to the sun, which feels like a missed opportunity.

I would like also to point out that part of what you suggested (and thank you for suggesting solutions 🙏) is currently possible with the current implementation of RiseTransitSet:

he = Astronoby::RiseTransitSet.new(
  observer: observer,
  date: date,
  coordinates_of_the_previous_day: coordinates_of_the_previous_day,
  coordinates_of_the_day: coordinates_of_the_day,
  coordinates_of_the_next_day: coordinates_of_the_next_day,
  additional_altitude: Angle.from_dms(0, 50, 0) # Sun's semi-diameter
)

he.times
he.azimuths
he.transit_altitude

I admit it's not perfect, having three different coordinates is important for close moving objects (by opposition with stars that look almost fixed within a few days) and the current API is not split into multiple methods like #rising_time.

[janfri]

How about having a "universal" class for "universal events" and a subclass for "events" of the sun:

class UniversalEvents
  attr_accessor :rising_time, :rising_azimuth, :transit_time, :transit_altitude, :setting_time, :setting_azimuth
end

class SunEvents < UniversalEvents
  attr_accessor :morning_civil_twilight_time, :evening_civil_twilight_time, :morning_nautical_twilight_time # ...
end

events1 = some_star.events(observer: observer)
events1.class # => UniversalEvents
events1.rising_time

events2 = sun.events(observer: observer)
events2.class # => SunEvents
events2.morning_civil_twilight_time
events2.rising_time

events3 = sun.events(observer: a_different_observer)
events3.class # => SunEvents
events3.morning_civil_twilight_time
events3.rising_time

The names of the classes "UniversalEvents" and "SunEvents" and the method name "events" are only to demonstrate my idea. I'm sure there are better alternatives.

This approach doesn't need any memoization for different observers because the data is stored in an explicit instance of UniversalEvents or SunEvents.

My main point is: use an object with clever method names instead of an array as return value of the method called with the sun (or other celestial body).

[janfri]

Btw. I'm only a weired Ruby guy who looks with a naive view at your api. But one who is developing in Ruby since over two decades. You are doing all the hard work with the maths. :)

[rhannequin]

I like this approach, @janfri!

I think I'm going to implement this in a follow-up PR as this one is already quite big and is required for other work. However I won't make a new version until this is fully worked out, so that hopefully the Sun's API is stable.

Btw. I'm only a weired Ruby guy who looks with a naive view at your api. But one who is developing in Ruby since over two decades. You are doing all the hard work with the maths. :)

I really appreciate you taking time to share your experience. Because this kind of work already exists in other languages, but not in Ruby, it is important to me to develop the library with quality Ruby code, following conventions and Ruby's spirit.
I might be less experienced, but what you suggested so far totally makes sense to me. It follows an object-oriented approach that is important in Ruby programming and it looks familiar to other gems I've explored.