astropy / astroplan

Observation planning package for astronomers – maintainer @bmorris3
https://astroplan.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
198 stars 109 forks source link

Hour angle constraint #560

Open mcoughlin opened 1 year ago

mcoughlin commented 1 year ago

This PR adds possibility of an hour angle constraint.

mcoughlin commented 1 year ago

Apologies @bmorris3. I fixed the linting issue.

bmorris3 commented 1 year ago

@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 commented 1 year ago

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

bmorris3 commented 1 year ago

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:

https://github.com/astropy/astroplan/blob/67c0c6ba8467fc26ca79a7cef056eb38a066abd2/astroplan/observer.py#L1917-L1944

mcoughlin commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

bmorris3 commented 1 year ago

@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 https://github.com/astropy/astroplan/pull/560#issuecomment-1604482430)?

mhvk commented 1 year ago

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 commented 1 year ago

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

mcoughlin commented 1 year ago

@bmorris3 maybe you can retrigger the tests?