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

Make DE 440(s) solar system ephemeris available #11601

Merged
merged 2 commits into from
Apr 19, 2021

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Apr 19, 2021

This ephemeris is the latest and presumably best. Right now, I stated that this would become the default in 5.0, but perhaps that should be 4.3? It is not a difficult change, but a lot of substitutions in our tests, so I thought I'd better ask before doing the work...

fixes #11544

@mhvk mhvk added this to the v4.3 milestone Apr 19, 2021
@pllim pllim requested review from adrn and eteq April 19, 2021 17:28
@pllim
Copy link
Member

pllim commented Apr 19, 2021

Does this mean we can revert #11564 or not?

Copy link
Member

@adrn adrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Indeed a simple update. I don't have strong feelings about making the default change now (for v4.3) vs. v5.0.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 19, 2021

OK, I updated the change-log fragment.

I had a quick look at trying to make de440s the default, but found rather many tests started failing (e.g., against the built-in ERFA ephemeris). So, probably best to push that change off and keep this simple.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 19, 2021

OK, tests passed. Once in, I'll put up a PR to change the default, just to show what changes are needed. That will be for 5.0, though.

Copy link
Member

@adrn adrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great LGTM, thanks!

@adrn
Copy link
Member

adrn commented Apr 19, 2021

Was about to hit merge, but then wondered: Do we want to add a test to make sure the new options work? I guess that would mean more remote-data...

@mhvk
Copy link
Contributor Author

mhvk commented Apr 19, 2021

@adrn - should have mentioned that - I indeed decided that checking locally was good enough in this case (which I did...). I'm happy at some point to add a test, but then we should use the cache prefill option that now exist so that we're not hammering the JPL servers...

@adrn
Copy link
Member

adrn commented Apr 19, 2021

OK, got it -- I'm fine with enabling this and merging given a local passing check, but perhaps make an issue to remind us to add (infrequently-run) tests to check all possible ephemerides?

@mhvk
Copy link
Contributor Author

mhvk commented Apr 19, 2021

Will do! (But will merge this first so I can have the draft PR with test changes out there too)

@mhvk mhvk merged commit 517961f into astropy:main Apr 19, 2021
@mhvk
Copy link
Contributor Author

mhvk commented Apr 19, 2021

See #11609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for the DE440 JPL ephemeris
4 participants