Add cached Sun/Moon calculations in Observer class
Introduction
I've been investigating the use of astroplan for the next major release of the API for NASA's Swift mission (https://www.swift.psu.edu/too_api/). This would involve some perhaps unusually intensive usage of astroplan, so I've been looking at small optimizations that can shave seconds off some API calls.
Reason for pull
astroplan makes use of the the astropyget_sun and get_body to calculate the position of the Moon and Sun. Unfortunately these calls are computationally expensive, which can lead to inefficiencies when calculating the positions of the Sun and Moon from scratch each time we need them. As the Sun/Moon positions are only unique for a given Observer location and set of times, it would be more efficient to cache them, rather than recalculating them every time we need them.
Solution
This pull request adds a get_body method to the Observer class, which calculates the position of any Solar System body supported by the astropyget_body, for the current Observer.location, at a given time. This method caches the results, using the same scheme as alt/az caching (reusing the _make_cache_key function , which I had to move to observer.py to avoid a circular import).
I then replaced all calls to get_sun and get_body in Observer and in constraints.py where the Observer class is used, with this method.
This ensures that for a given Observer and time, the positions of the Sun and Moon are only ever calculated once. In my own code, this results in a 4x speedup when calculating multiple target visibilities, after an initial calculation.
Possible issues
Replacement of get_sun calls
I replaced get_sun with Observer.get_body('sun', ...) calls, which means that get_sun calls that previously did not pass the location parameter, now do. I do not believe this will cause any problems, and should be both negligibly different and "more correct". However, there is a possibility I misunderstood the reason for not passing location to get_sun in some cases. The solution to this would be to create a second get_sun method that mimics exactly the previous use, but with caching.
To investigate this, I used the Observer.altaz method to calculate the Alt/Az of the Sun, for the example given in the inline code comment. The use of get_body with location set instead of a plain get_sun does result in different values, but it differs by a negligible amount (angular separation of 0.005 arc-seconds between the two values).
I will note that my testing/development of this pull request has been done exclusively with astropy 6.0.
The get_body method name
Stylistic choice, but it might be better to use a different method name than the astropy get_body for this, to avoid confusion? I wavered between get_body and _get_body.
Add cached Sun/Moon calculations in
Observer
classIntroduction
I've been investigating the use of
astroplan
for the next major release of the API for NASA's Swift mission (https://www.swift.psu.edu/too_api/). This would involve some perhaps unusually intensive usage ofastroplan
, so I've been looking at small optimizations that can shave seconds off some API calls.Reason for pull
astroplan
makes use of the theastropy
get_sun
andget_body
to calculate the position of the Moon and Sun. Unfortunately these calls are computationally expensive, which can lead to inefficiencies when calculating the positions of the Sun and Moon from scratch each time we need them. As the Sun/Moon positions are only unique for a given Observer location and set of times, it would be more efficient to cache them, rather than recalculating them every time we need them.Solution
This pull request adds a
get_body
method to theObserver
class, which calculates the position of any Solar System body supported by theastropy
get_body
, for the currentObserver.location
, at a giventime
. This method caches the results, using the same scheme as alt/az caching (reusing the_make_cache_key
function , which I had to move toobserver.py
to avoid a circular import).I then replaced all calls to
get_sun
andget_body
inObserver
and inconstraints.py
where theObserver
class is used, with this method.This ensures that for a given
Observer
andtime
, the positions of the Sun and Moon are only ever calculated once. In my own code, this results in a 4x speedup when calculating multiple target visibilities, after an initial calculation.Possible issues
Replacement of
get_sun
callsI replaced
get_sun
withObserver.get_body('sun', ...)
calls, which means thatget_sun
calls that previously did not pass thelocation
parameter, now do. I do not believe this will cause any problems, and should be both negligibly different and "more correct". However, there is a possibility I misunderstood the reason for not passing location toget_sun
in some cases. The solution to this would be to create a secondget_sun
method that mimics exactly the previous use, but with caching.To investigate this, I used the
Observer.altaz
method to calculate the Alt/Az of the Sun, for the example given in the inline code comment. The use ofget_body
withlocation
set instead of a plainget_sun
does result in different values, but it differs by a negligible amount (angular separation of 0.005 arc-seconds between the two values).I will note that my testing/development of this pull request has been done exclusively with
astropy
6.0.The
get_body
method nameStylistic choice, but it might be better to use a different method name than the astropy
get_body
for this, to avoid confusion? I wavered betweenget_body
and_get_body
.