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

Add cached Sun/Moon calculations in Observer class #578

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jak574
Copy link

@jak574 jak574 commented Jan 20, 2024

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 astropy get_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 astropy get_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.

@jak574
Copy link
Author

jak574 commented Jan 20, 2024

OK, I see a test failed that didn't happen in my checks. I can look into this.

import order fix

Add back missing import

Add back another missing import

Move _make_cache_key to avoid circular import

Formatting fix

Fix quote consistency

Consistency fixes

Minor fix to _make_cache_key to fix test crash
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

Successfully merging this pull request may close these issues.

1 participant