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

Hour angle constraint #560

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

mcoughlin
Copy link

This PR adds possibility of an hour angle constraint.

@mcoughlin
Copy link
Author

Apologies @bmorris3. I fixed the linting issue.

@bmorris3
Copy link
Contributor

@mcoughlin Just a few more:

astroplan/constraints.py:947:69: W291 trailing whitespace
astroplan/constraints.py:948:26: W291 trailing whitespace
astroplan/tests/test_constraints.py:168:34: W291 trailing whitespace

@mcoughlin
Copy link
Author

@bmorris3 I think I got those in the last commit.

@bmorris3
Copy link
Contributor

Hi @mcoughlin,

Thanks for starting this constraint.

In astroplan we try to use full-precision astropy methods wherever possible, so that results from astroplan will agree with doing the calculation "the long way" via astropy. Astroplan already has a method for computing hour angle, so an hour angle constraint should use the result of this method:

def target_hour_angle(self, time, target, grid_times_targets=False):
"""
Calculate the local hour angle of ``target`` at ``time``.
Parameters
----------
time : `~astropy.time.Time` or other (see below)
Time of observation. This will be passed in as the first argument
to the `~astropy.time.Time` initializer, so it can be anything that
`~astropy.time.Time` will accept (including a `~astropy.time.Time`
object)
target : `~astropy.coordinates.SkyCoord`, `~astroplan.FixedTarget`, or list
Target celestial object(s)
grid_times_targets: bool
If True, the target object will have extra dimensions packed
onto the end, so that calculations with M targets and N times
will return an (M, N) shaped result. Otherwise, we rely on
broadcasting the shapes together using standard numpy rules.
Returns
-------
hour_angle : `~astropy.coordinates.Angle`
The hour angle(s) of the target(s) at ``time``
"""
time, target = self._preprocess_inputs(time, target, grid_times_targets)
return Longitude(self.local_sidereal_time(time) - target.ra)

@mcoughlin
Copy link
Author

@bmorris3 We tried that and found the calculation quite slow for constraints. Maybe it makes sense to just have a warning in the doc string?

@bmorris3
Copy link
Contributor

I wonder why it's so slow. Is it possible that a different combination of arguments in the local_sidereal_time method would be faster? https://docs.astropy.org/en/stable/time/index.html#sidereal-time-and-earth-rotation-angle

@mcoughlin
Copy link
Author

@bmorris3 Not sure. I just fixed the PR to use Astropy by default though.

@bmorris3
Copy link
Contributor

@mhvk We're using astropy.time.Time.local_sidereal_time to compute LST in astroplan:

https://github.com/astropy/astroplan/blob/67c0c6ba8467fc26ca79a7cef056eb38a066abd2/astroplan/observer.py#L1914C29-L1915

Do you know if there are more performant options to give the LST functions, so that we can compute many accurate LSTs quickly (see #560 (comment))?

@mhvk
Copy link

mhvk commented Jun 26, 2023

If you don't need full precision, you can avoid the correction for the TIO relative to longitude 0. Easiest is picking an old model, "iau1982", which doesn't have it -- this is a factor 4 faster and seems accurate to better than 4e-7 hours.

More accurate (1e-10 hours) and almost as fast is

time.sidereal_time(kind, longitude='tio', model=model) + self.location.lon

For planning purposes, either seems reasonable.

p.s. We probably should have something in our documentation about the speed differences...

@mcoughlin
Copy link
Author

Thanks @mhvk ! @bmorris3 I gave that a whirl.

@mcoughlin
Copy link
Author

@bmorris3 maybe you can retrigger the tests?

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.

None yet

3 participants