Add cached Sun/Moon calculations in Observer
class
#578
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.