Add gaiatap module in order to access to the European Space Agency Gaia Archive #836

jcsegovia · 2017-01-27T09:02:14Z

I am Juan-Carlos Segovia, a software engineer working for the European Space Agency.
(You can contact me at: juan.carlos.segovia@sciops.esa.int or juancarlos.segovia@gmail.com)

The ESAC Space Data Centre (ESDC) has developed a module to access the European Space Agency Gaia Archive.

We would like to add this module to astroquery library.

Gaia Archive access is based on a TAP+ REST service.

TAP+ is an extension of Table Access Protocol (TAP: http://www.ivoa.net/documents/TAP/) specified by the International Virtual Observatory Alliance (IVOA: http://www.ivoa.net).

The gaiatap module allows the access to the archive using astroquery API (query_object), and also, provides some TAP+ capabilities.

Please, will you be so kind to review and check gaiatap module?

Thank you very much.

Best regards,

Juan-Carlos Segovia Serrato

ESAC Science Data Centre
Science Archives and Computer Engineering Unit
Operations Department, Directorate of Science
European Space Astronomy Centre (ESAC)
European Space Agency (ESA)

email: juan.carlos.segovia@sciops.esa.int
Phone: (34)-91-8131-175 - 70175 internal
Fax: (34)-91-8131-308

European Space Astronomy Centre (ESAC)
Camino Bajo del Castillo s/n
Urb. Villafranca del Castillo
28692 Villanueva de la Cañada, Madrid, Spain.

bsipocz · 2017-01-27T12:52:21Z

Thanks you @jcsegovia for this awesome contribution. We'll try to review it in details soon.

bsipocz · 2017-01-27T13:14:02Z

I'm pinging @eteq and @andycasey as participants of previous discussions about Gaia TAP.

bsipocz · 2017-01-27T13:32:38Z

A few quick, mainly stylistic comments I have to start with.

I think the example directories are superfluous at their current place. However a narrative documentation is missing. Could you rework it under docs/gaiatap with a little bit more narrative text? One good examples is e.g. the docs/esasky/esasky.rst.

We follow the naming conventions of functions and methods are lower case (in the tests, too) while classes are CamelCase.

We have our testing suite set up with py.test. While unittest is compatible with it, I see issues when using python 3.6. It's basically only the esasky module that use unittest and as a workaround for the py3.6 issue, I'm planning to refactor it to be pure pytest. If it's not too controversial, would it be possible to rework the tests here, too to be pure pytest?

bsipocz · 2017-01-27T13:38:37Z

Oh, and I think it would make sense to rename the module to gaia. Any tap functionality then can be factored out either into gaia.utils, or to a more generic astroquery.utils.tap location, so other modules could possibly use them. (However that all that refactoring be part of a future PR).

jcsegovia · 2017-01-27T13:50:49Z

Thanks.

Currently, we have created gaiatap/gaiatap.rst with the explanations. I think it should be moved to docs/gaia/gaia.rst

Changes:

rename gaiatap to gaia
create docs/gaia/gaia.rst (move current gaiatap/gaiatap.rst to docs/gaia/gaia.rst)
follow python naming convention for tests
unittest -> pytest

Question: shall we change 'private' method names also?

About TAP: fine with us to move to a more generic place.

jcsegovia · 2017-01-27T13:59:27Z

Sorry, I forgot unittest:

OK, we can remove unittest, but it will take a bit of time because we are using some mocking capabilities.

bsipocz · 2017-01-27T14:25:54Z

@jcsegovia - Thanks, it sounds good. Before you do much work on it, it may be worth waiting for comments from @keflavich, he may have a few generic points too that I missed.

keflavich

A few minor comments.

keflavich · 2017-01-27T15:28:13Z

astroquery/gaiatap/core.py

+        return self.__gaiatap.query_object_async(coordinate, radius=radius, width=width, height=height, verbose=verbose)
+
+    def query_region(self, coordinate, radius=None, width=None):
+        print ("Not available")


For these 'not available' methods, could you just leave them commented out for now? I wouldn't want users tricked into thinking that method is available when it's not yet.

Also, we prefer to raise NotImplementedError() rather than print a statement like this

keflavich · 2017-01-27T15:28:44Z

astroquery/gaiatap/core.py

+        -------
+        A Job object
+        """
+        return self.__gaiatap.launch_async_job(query, name=name, output_file=output_file, output_format=output_format, verbose=verbose, dump_to_file=dump_to_file, background=background, upload_resource=upload_resource, upload_table_name=upload_table_name)


Minor style issue: could you make sure these long lines are wrapped at 80 characters, or at least close to 80?

keflavich · 2017-01-27T15:30:09Z

astroquery/gaiatap/example/example.py

+if jobs is None:
+    print ("No jobs")
+else:
+    for j in jobs:


Could you just print(jobs) here? Or is the formatting ugly in this case?

keflavich · 2017-01-27T15:31:24Z

astroquery/gaiatap/tapplus/conn/tests/DummyConnHandler.py

+    #python 2
+    from urllib import urlencode as urlEncode
+
+class DummyConnHandler:


this should probably be class DummyConnHandler(object):

keflavich · 2017-01-27T15:31:50Z

astroquery/gaiatap/tapplus/conn/tests/setup_package.py

+"""
+import os
+
+def get_pakcage_data():


pakcage->package

keflavich · 2017-01-27T15:32:37Z

astroquery/gaiatap/tapplus/conn/tests/test_conn.py

+        assert r.getMethod() == 'POST', "Request method. Expected %s, found %s" % ('POST', r.getMethod())
+        assert r.getContext() == context, "Request context. Expected %s, found %s" % (context, r.getContext())
+        assert r.getBody() == data, "Request body. Expected %s, found %s" % (data, str(r.getBody()))
+        pass


a pass statement here and in other surrounding functions is unnecessary

keflavich · 2017-01-27T15:33:50Z

astroquery/gaiatap/gaiatap.rst

+---------------------------
+
+Authenticated users are able to access to TAP+ capabilities (shared tables, persistent jobs, etc.)
+In order to authenticate a user, ``login`` or ``login_gui`` methods must be called. After a successful


You should include a note here that Tkinter is required for the login gui

keflavich · 2017-01-27T15:37:44Z

@jcsegovia Thanks for this contribution, it is awesome! I've left a handful of minor comments above.

@bsipocz's comments were spot on - we would certainly prefer to stick with an all-pytest framework if that is possible. In particular, I couldn't quite tell from skimming the code - do any of your tests require a remote connection? If so, we need to disable those by default and only enable them when the --remote-data flag is set.

I won't be able to do a complete review of this module, as it is huge and has a lot of capabilities I don't fully understand. But, I'm happy to have it!

keflavich · 2017-01-27T15:38:55Z

@jcsegovia I edited your comment above to have a checklist for the items to be changed.

jcsegovia · 2017-01-27T16:04:31Z

@bsipocz, @keflavich: thank you very much for your inputs.

About remote connection for tests: no we do not need it. Out tests are run in a isolated (mocked) environment.

We are working on the implementation of these changes...

Thanks again!

…class)

jcsegovia · 2017-03-13T14:58:32Z

@bsipocz , @keflavich where do you want me to put tap.rst? Currently, it is under astroquery/docs/tap/tap.rst (I haven't seen an astroquery/docs/utils section...)

Also, in which index.rst section shall I put tap?
Shall I modify utils.rst in some way...?

keflavich · 2017-03-13T15:05:28Z

Good questions. I think docs/tap/tap.rst is fine for now. Add a link to it in utils.rst as well as on the main index. Filling out utils.rst is a big to-do item.

bsipocz · 2017-03-13T15:05:43Z

@jcsegovia - Probably the index page could use a bit of reorganizing, until that happens tap could go under "Other" I think: http://astroquery.readthedocs.io/en/latest/#other

bsipocz · 2017-03-13T15:09:21Z

Also I've just noticed that all the svn files are getting included.
However we can remove them once this is merged and make sure they will never get back (doesn't matter whether you do it in this PR or we do separately as they are already in the history (as unless there is a large rebase/squash operation done which could be messy at this time, so I don't recommend it)).

jcsegovia · 2017-03-13T15:16:52Z

@bsipocz .svn entries: my fault (sorry). They are removed.

bsipocz · 2017-03-14T01:50:52Z

@jcsegovia - I've pushed directly to this branch a few tweaks to the organization of the docs pages to make sure the tests pass. Once CI is all green, I'll merge.

bsipocz · 2017-03-14T02:09:45Z

Thank you @jcsegovia again for this contribution!

jcsegovia · 2017-03-14T09:26:02Z

@bsipocz , @keflavich thank you for your help and support.

jcsegovia added 2 commits January 26, 2017 16:57

Added gaiatap

827f2c4

Added CHANGES.rst file

d8da90d

bsipocz added the New Service label Jan 27, 2017

Changed import paths

d94d259

jcsegovia closed this Jan 27, 2017

jcsegovia reopened this Jan 27, 2017

bsipocz added the gaia label Jan 27, 2017

keflavich reviewed Jan 27, 2017

View reviewed changes

bsipocz and others added 12 commits January 27, 2017 17:00

Fixing typo in setup_package files and adding missing content types

552624d

Added docs/gaia/gaia.rst

8eaab94

Removed gaiatap.rst (moved to ../docs/gaia/gaia.rst)

1f0da48

Updated CHANGES.rst (docs/gaia/gaia.rst)

142f7b4

Updated examples: job loop changed by print(jobs)

842f63f

Added missing extension object to several classes

efd0860

Changed DummyTapHandler method names

131637e

Changed method names

3ba658c

Changed method names

8510166

Changed method names DummyConnHandler

1d8475e

Updated method names DummyResponse

6836a25

Changed method names test_conn

e1ce009

jcsegovia added 12 commits March 13, 2017 09:12

Removed EsaSky reference

055917a

Removed CHANGES.rst file

9bd698d

Removed 'Job' import from gaia package

eceaad0

Replaced sys.version_info by six

06535df

Renamed tap/column.py to tap/tapcolumn.py

6a83920

Added tapcolumn.py

cc67e72

Renamed tap/table.py to tap/taptable.py (Table class to TapTableMeta …

3b1815b

…class)

Added new package utils/tap

be41b15

Removed unused example directory

dc492a7

Updated tap.rst imports

dcb21f9

Updated gaia package to use utils/tap (instead of tap)

7ddaee8

Remove tap package (new one: utils/tap)

47d20f5

Removed .svn entries

8b8b895

jcsegovia and others added 4 commits March 13, 2017 16:41

Updated index.rst and utils.rst

2134a18

Minor tweaks to the placing on the index page

9e5785f

Adding svn to gitignore

61703e2

Fixing various docs warnings

9cb4832

bsipocz merged commit 8dc7547 into astropy:master Mar 14, 2017

bsipocz added this to the 0.3.5 milestone Mar 14, 2017

keflavich mentioned this pull request Apr 17, 2018

Some TAP jobs are stuck at PENDING #1089

Closed

This was referenced Nov 7, 2019

TAP/TAP+ doesn't support the MAXREC parameter #1581

Closed

TAP: add MAXREC support (#1581) #1584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gaiatap module in order to access to the European Space Agency Gaia Archive #836

Add gaiatap module in order to access to the European Space Agency Gaia Archive #836

jcsegovia commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

jcsegovia commented Jan 27, 2017 •

edited by keflavich

jcsegovia commented Jan 27, 2017

bsipocz commented Jan 27, 2017

keflavich left a comment

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich Jan 27, 2017

keflavich commented Jan 27, 2017

keflavich commented Jan 27, 2017

jcsegovia commented Jan 27, 2017

jcsegovia commented Mar 13, 2017

keflavich commented Mar 13, 2017

bsipocz commented Mar 13, 2017

bsipocz commented Mar 13, 2017

jcsegovia commented Mar 13, 2017

bsipocz commented Mar 14, 2017

bsipocz commented Mar 14, 2017

jcsegovia commented Mar 14, 2017

Add gaiatap module in order to access to the European Space Agency Gaia Archive #836

Add gaiatap module in order to access to the European Space Agency Gaia Archive #836

Conversation

jcsegovia commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

bsipocz commented Jan 27, 2017

jcsegovia commented Jan 27, 2017 • edited by keflavich

jcsegovia commented Jan 27, 2017

bsipocz commented Jan 27, 2017

keflavich left a comment

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich Jan 27, 2017

Choose a reason for hiding this comment

keflavich commented Jan 27, 2017

keflavich commented Jan 27, 2017

jcsegovia commented Jan 27, 2017

jcsegovia commented Mar 13, 2017

keflavich commented Mar 13, 2017

bsipocz commented Mar 13, 2017

bsipocz commented Mar 13, 2017

jcsegovia commented Mar 13, 2017

bsipocz commented Mar 14, 2017

bsipocz commented Mar 14, 2017

jcsegovia commented Mar 14, 2017

jcsegovia commented Jan 27, 2017 •

edited by keflavich