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

Provide for a cleaner API for querying NASA archive #1606

Merged
merged 3 commits into from Dec 30, 2019

Conversation

sashank27
Copy link
Contributor

As per the discussions in #1351 and #1605, refactor the NASA exoplanet API to query for a specific planet (using query_planet()) as well as for host stellar system (using query_star()), using their API.
This prevents unnecessary hacking with the downloaded full table.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

Please add tests to replace those removed.

@pep8speaks
Copy link

pep8speaks commented Dec 27, 2019

Hello @sashank27! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-12-30 17:03:30 UTC

@codecov
Copy link

codecov bot commented Dec 27, 2019

Codecov Report

Merging #1606 into master will decrease coverage by 0.07%.
The diff coverage is 32.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
- Coverage   62.91%   62.83%   -0.08%     
==========================================
  Files         185      185              
  Lines       14956    14976      +20     
==========================================
+ Hits         9409     9410       +1     
- Misses       5547     5566      +19
Impacted Files Coverage Δ
...y/nasa_exoplanet_archive/nasa_exoplanet_archive.py 64.06% <32.25%> (-26.85%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56025c6...f7dadcf. Read the comment docs.

@keflavich
Copy link
Contributor

lgtm! If you want to add a changelog entry, I'll merge this afterward, otherwise I'll merge next time I have access to command line (I'm writing this from mobile, so I can't easily add the changelog entry myself now)

@sashank27
Copy link
Contributor Author

@keflavich what about the coverage? Also, do @bsipocz also need to review this?

@keflavich
Copy link
Contributor

Coverage isn't an issue; we generally see reduction in coverage for any new features. We don't generally require multiple reviews.

@keflavich keflavich merged commit e52c7ed into astropy:master Dec 30, 2019
@sashank27
Copy link
Contributor Author

@keflavich @bsipocz thanks for the support!

@sashank27 sashank27 deleted the exoplanet-api branch December 30, 2019 19:58
@bmorris3
Copy link
Contributor

Hi all – this PR is causing backwards compatibility failures because it removes the get_confirmed_planets_table function. Was that intentional?

@keflavich
Copy link
Contributor

If I understand correctly, get_confirmed_planets_table retrieved the whole table? I think this should be deprecated, but should not have simply been removed.

Can we re-implement get_confirmed_planets_table as a wrapped of query_planets, but with a deprecation warning suggesting that you should use the newer syntax?

@bsipocz bsipocz added this to the v0.4 milestone May 4, 2021
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.

None yet

5 participants