Exoplanet catalog queries - #771

(1) seems fine
(2) Not necessarily, since they are independent entities. Vizier and IRSA have some overlap (e.g., both contain the 2MASS and IRAS catalogs). However, it would be helpful to note & describe the overlap.
(3) these should probably be in astroquery/exoplanets/data instead

I've moved the data to astroquery/exoplanets/data.

I think the astropy lts builds are failing because I use units in the exoplanets table that were recently introduced to astropy (astropy/astropy#4818). Which version is lts?

How do you think I should I approach this – should we mark the exoplanets tests only for astropy > 1.2? cc @bsipocz

@bmorris3 yes, have the tests that rely on that check the astropy version. Maybe also add a check in the module itself to raise a NotImplementedError if the astropy version is too low

Update on the docs page to include the PlanetParams.coord docs:

The last remaining build failure is a sphinx warning about references:

<autosummary>:1: WARNING: citation not found: R1
<autosummary>:1: WARNING: citation not found: R3
<autosummary>:1: WARNING: citation not found: R15
<autosummary>:1: WARNING: citation not found: R16

Do you know if I am using url references incorrectly?

I think the errors remaining at this point are raised by another part of astroquery due to the dev version of astropy.

Looks good. Are you ready for it to be merged?

I've seen those failures on other builds too. I think @mhvk mentioned this on astropy-dev. This thread is supposed to answer appveyor fails, but we're seeing travis fails, not appveyor...

Looks good. Are you ready for it to be merged?

I think so, but I'd love your comments on the docs. Are there sufficient examples? I wanted to show some more outputs, but there are tons of columns in these tables.

The failures look real, though would seem astropy.wcs related, with the views passed in not converted to int when turned into array indices (which has become required in numpy relatively recently). Would be good to raise this issue on astropy itself (ideally with a minimal example, of course...)

Ahh, sorry, the errors are different than last time I looked; previously it was an install error affecting these two versions. I suspect it's related to this: astropy/astropy#5411 but I'll investigate more.

@bmorris3 if you rebase, it should fix the build.

More examples are always good, but I can't suggest what to add.

@bmorris3 - would you mind rebasing again? It seems that quite a few unrelated commits are picked up during the last one.

@bsipocz What could I have done wrong? I did:

git fetch upstream
git rebase upstream/master

Now when I do that command a second time I get tons of merge conflicts which are just previous commits.

@bmorris3 - I'm heading home, but will definitely look at it later tonight.
My first guess is that you see more if you do an interactive rebase, at least easier to spot where it goes wrong.

git rebase -i upstream/master is what I do. I don't know what happened to yours, and I'm not sure a different rebase approach will fix it. Rebasing is a tricky art.

Ahh, sorry, the errors are different than last time I looked; previously it was an install error affecting these two versions. I suspect it's related to this: astropy/astropy#5411 but I'll investigate more.

I haven't realized when making that PR that it imposes another restriction. Could you raise an issue in astropy and cc me?

Hi @bmorris3, thanks a lot for working on this! 👏 Simplifying access to these two exoplanet archives is a valuable thing to do. I believe this would be a hot 🔥 feature that would get a lot of use.

I found time to play with astroquery.exoplanets in detail today. I identified six issues which I hope you may want to consider. Apologies in advance if these have already been discussed elsewhere. (I also include a few trivial comments at the end.)

Issues:

1. Is there a reason to have this code live under astroquery.exoplanets as opposed to adding the modules astroquery.exoplanet_archive and astroquery.exoplanets_org? I say this because all existing modules in astroquery seem to represent a data provider rather than a field of science. In fact there is already a module called astroquery.open_exoplanet_catalogue which will be confusing to have alongside astroquery.exoplanets.
2. I believe you forgot to list astroquery.exoplanets under "Available Services" on the front page of the docs. It is listed further down under "Catalogs" in the by-type listing, which I find confusing because this is an interface to two archives and so I would have looked for it under "Archives". Of course this relates to question 1.
3. Why is the user required to call PlanetParams.from_exoplanet_archive() rather than ExoplanetArchive.get_planet(). Is there a rationale behind this design decision? The latter may be preferred because it reduces the number of different classes a user needs to interact with, and matches the concept of having users interact with a "service" rather than a "type of science". I certainly haven't thought carefully about this however. Do other parts of astroquery have e.g. a 'star' or 'galaxy' object that users query with?
4. The NASA Exoplanet Archive serves 26 different tables altogether, including e.g. planet candidates from different missions, but ExoplanetArchive.get_table() only returns one specific table (i.e. the table of confirmed planets). We should consider renaming get_table() into e.g. get_exoplanets_table() to make this explicit, or alternatively we should accept the table name as a parameter to enable the other 25 tables to be accessed as well. The confirmed planets table could of course be the natural default.
5. The exoplanet archive only returns a subset of columns from its tables by default. One needs to add &select=* to the API call to get all columns (or &select=name1,name2,... for a subset). It would be nice if ExoplanetArchive.get_table() had an option to specify the columns desired.
6. It would be helpful for the docs to contain a direct link to the page at the NASA Exoplanet Archive that documents the exact meaning of each column in the confirmed exoplanets table (URL: http://exoplanetarchive.ipac.caltech.edu/docs/API_exoplanet_columns.html ). If we decide to support other tables as well, then we should also link to the page that explains that lists them by name (URL: http://exoplanetarchive.ipac.caltech.edu/docs/program_interfaces.html#data ).

Finally, some minor suggestions that are trivial:

• Please consider adding an __all__ to exoplanets/__init__.py. It would prevent the objects that are being imported from __future__ from being shown in the exoplanets namespace, which makes exploring the functionality from the command line easier.
• Consider adding a short docstring to the constructors of the ExoplanetArchive and ExoplanetsOrg classes, also to aid command line use.
• The api is intolerant of white spaces and dashes in the planet names. For example, PlanetParams.from_exoplanet_archive('Tres-2 b') works but with 'Tres-2b' or 'Tres2b' it doesn't. Do we want to enforce a standard, or can we make it more lenient?
• The error message returned by e.g. ``PlanetParams.from_exoplanet_archive('Tres2b') isValueError: Planet "{0}" not found in exoplanets.org catalog`. This contains two bugs: `{0}` and `exoplanets.org`.

I hope this is useful feedback. Thanks again for working on this.

@barentsen Good points, especially about splitting into two modules. I think we could retain exoplanets as a host for the various objects (e.g., PlanetParams) but have separate modules for each of the individual archive services. @bmorris3, how does that sound to you? @barentsen, if @bmorris3 is onboard, would you want to contribute some of those changes? (PR on his PR)

@bmorris3 - Re to the rebase (sorry it took so long to get back to this):

For some reason the commits are doubled in this branch. I'm not sure what went wrong and where, but the fix is easy:

• Do an interactive rebase git rebase -i astroquery/master. This will bring up your favourite editor with these commits:

pick c78df27 Init exoplanets module commit
pick d3aba17 New exoplanet parameter query API
pick 47f6ace Moving exoplanets units data to exoplanets/data, removing doctests.
pick 41b2e3e Adding package data setup_package script
pick 8370867 Adding minversion requirement for R_jup tests, adding coordinates
pick 04a55d1 Adding docs for the coord property [ci skip]
pick 5b8064e change the links
pick 0826bfe Moving to OO-style table caching
pick a156b02 Adding astroquery-style query_planet method
pick bf331c8 Adding more docs for downloading/querying tables
pick 211f142 more robust handling of units for astropy<1.2
pick 4ff784e Renaming test file
pick 7258f08 Init exoplanets module commit
pick 298d591 New exoplanet parameter query API
pick 61b8e7b Moving exoplanets units data to exoplanets/data, removing doctests.
pick 5bc64a2 Adding package data setup_package script
pick abb834f Adding minversion requirement for R_jup tests, adding coordinates
pick f55000e Adding docs for the coord property [ci skip]
pick aba04a5 change the links
pick 7621949 Moving to OO-style table caching
pick 566be7b Adding astroquery-style query_planet method
pick 43c4c1e Adding more docs for downloading/querying tables
pick ff229e1 more robust handling of units for astropy<1.2
pick afba8d7 Renaming test file

Simply keep only the first 12, delete the rest as they are pure repeats. I've checked, the commits are the same as the ones above them. Once doing that, there will be no conflicts, etc, and you will only need to force push it back here. If you want, you can check my copy of this branch, rebased here: https://github.com/bsipocz/astroquery/tree/bmorris3_exoplanets

@bmorris3 - any chance you find time to finish this up? I would love to use it :)

@bsipocz It's probably going to be a while – I'm slammed until after AAS 😢

Hi there @bmorris3 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I see this is a work in progress pull request. I'll report back on the checks once the PR is ready for review.

Hello @bmorris3! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on October 02, 2017 at 16:07 Hours UTC

Thanks @bmorris3 to pickig this up again. I have a few minor comments now, but will do a full review once you're ready.

• please rebase, there shouldn't be any merge commits or changes to astropy_helpers, and the commit duplicates are also still around
• I wonder whether we could come up with a less informal name for exoplanets_org, something that better reflects the underlying data of "Exoplanet Orbit Database"

Hey @barentsen and @keflavich, I'm finally following up on Geert's awesome code review. I'm going to write up some notes on what I've done:

1. I've split up the exoplanets module into three modules. There's an exoplanets module which houses the PlanetParams object, and could be the future home to any database-independent planet operations.

2. I've added exoplanets under available services.

3. The rationale behind PlanetParams.from_exoplanet_archive() rather than ExoplanetArchive.get_planet() gets a bit philosophical. Typically when you create a new instance of an object, you already know what attributes will be created on the new object. However, PlanetParams populates its attributes depending on what data source you give it. Since you can only know the attributes of a given PlanetParams object if you know where it came from, I wanted to make it explicit from what conditions you created each PlanetParams object. By making the constructor a class-method on PlanetParams, it ensures that the code that creates a new PlanetParams object always points back to its source.

4. I see how that could be confusing. I've changed the name from get_table to get_confirmed_planets_table to make it clearer.

5. I will welcome your PR making this change in the future!

6. A direct link to the Exoplanet Archive column description has been added.

@barentsen / @keflavich Do you think you'll have a chance to review the changes sometime soon? I'd like to make a major update to astroplan at some point in the near future which will depend on this update to astroquery. Thanks again!

@bsipocz Haha! I think I got it.

Looks good, though we still have 2 doc warnings:
https://travis-ci.org/astropy/astroquery/jobs/282057665#L2020

@keflavich All fixed.

I added a tiny changelog entry for this in 79397ec

Thanks again for the extensive reviews, @barentsen!