Skip to content

Feat/get moon - #4724

Closed
manikanta-dornala wants to merge 2 commits into
astropy:masterfrom
manikanta-dornala:feat/get_moon
Closed

Feat/get moon#4724
manikanta-dornala wants to merge 2 commits into
astropy:masterfrom
manikanta-dornala:feat/get_moon

Conversation

@manikanta-dornala

@manikanta-dornala manikanta-dornala commented Mar 23, 2016

Copy link
Copy Markdown

Implementing get_moon using SOFA Library.
A lot of work is to be be done in coordinating the Fundamental arguments of moon.
I made this pull request as per your guidelines to apply for GSOC.

@pllim pllim added coordinates Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 23, 2016
@pllim pllim added this to the v1.2.0 milestone Mar 23, 2016
@pllim

pllim commented Mar 23, 2016

Copy link
Copy Markdown
Member

Some comments:

  • Add a mention of which issue number this is addressing
  • Add change log
  • Add test
  • Fix existing tests that failed

@eteq

eteq commented Mar 24, 2016

Copy link
Copy Markdown
Member

Also, this is definitely not correct. It appears you're interpreting the output of faf03 as the latitude in GCRS. this is definitely not right, as faf03 outputs "mean longitude of the Moon minus mean longitude of the ascending node". That's not the GCRS latitude, it's a longitude.

I suggest you have a look at the wikipedia article on orbital elements, and maybe also the IERS standards document (although that is admittedly very dense)...

@pllim pllim mentioned this pull request Mar 25, 2016
Closed
@pllim pllim removed this from the v1.2.0 milestone Mar 25, 2016
@eteq

eteq commented Mar 28, 2016

Copy link
Copy Markdown
Member

cc @bmorris3, as he has a partial (or maybe complete?) version of get_moon in astroplan - not the same implementation, but still useful for comparison.

@eteq

eteq commented Mar 28, 2016

Copy link
Copy Markdown
Member

See the discussion in #4736 (comment) for a bit more on how this might be fixed.

@bmorris3 bmorris3 mentioned this pull request Mar 29, 2016
Closed
@StuartLittlefair StuartLittlefair mentioned this pull request May 13, 2016
Merged
@bsipocz

bsipocz commented May 14, 2016

Copy link
Copy Markdown
Member

This can be closed since #4890 is merged.

@mhvk

mhvk commented May 14, 2016

Copy link
Copy Markdown
Contributor

Indeed, will close this. Thanks @manikantareddyd for the trial.

@mhvk mhvk closed this May 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release coordinates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@manikanta-dornala @pllim @eteq @bsipocz @mhvk