Skip to content

Conversation

bmorris3
Copy link
Contributor

@bmorris3 bmorris3 commented Aug 4, 2016

This PR removes the location keyword argument from the moon_illumination, moon_phase_angle functions in the moon module.

The motivation here is that astropy's get_moon does not yet support computations for multiple times at a non-geocentric observer, so we were calculating each moon position with a slow for loop (astropy/astropy#5216). While the location of the observer on the surface of the Earth matters a lot if you want precise moon positions on the celestial sphere, it doesn't affect the result much for computing moon phase/illumination compared to a geocentric observer. So we can get vast speed-ups if we just drop the location specification here, where it doesn't matter much.

I've simply removed the location keyword argument, which now computes all phases/illuminations from GCRS=(0, 0, 0).

cc @StuartLittlefair @eteq @kvyh

@StuartLittlefair
Copy link
Contributor

HI @bmorris3

I've left a comment on astropy/astropy#5216, but there's a PR for astropy which will fix that issue. That PR has a milestone of v1.2.2. I guess you might want to merge this anyway if you want the next astroplan release to support astropy 1.2 and 1.2.1...

@bmorris3
Copy link
Contributor Author

bmorris3 commented Aug 4, 2016

I'm going to wait until at least tomorrow to merge. This should also help speed up your constraint computations when the moon illumination is evaluated.

@kvyh kvyh mentioned this pull request Aug 4, 2016
@bmorris3 bmorris3 merged commit 235db81 into astropy:master Aug 5, 2016
@jcurbo
Copy link

jcurbo commented Jan 20, 2017

Hi, it looks like when you removed the location requirement you didn't update the docstrings, so the generated docs (see here) still say you need to include a location. Looks like a straightforward change (remove two lines).

Also the same for moon_phase_angle.

@bsipocz bsipocz mentioned this pull request Feb 2, 2017
@bsipocz
Copy link
Member

bsipocz commented Feb 2, 2017

@jcurbo - Thanks for reporting this. #284 addresses the issue.

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.

4 participants