Add VO Cone Search by pllim · Pull Request #859 · astropy/astroquery

pllim · 2017-03-18T02:10:56Z

This is the first step towards astropy/astropy#5558.

If this is merged soon enough, maybe on the Astropy side, we can start issuing deprecation warnings in v2 and completely remove it in v3, but that discussion belongs in Astropy.

bsipocz · 2017-03-18T02:18:56Z

@pllim - I'm putting this into the new 0.3.6 milestone, as 0.3.5 is almost out of the door (pending only #855 I believe).

pllim · 2017-03-18T02:23:10Z

That is understandable. Any date planned for 0.3.6?

bsipocz · 2017-03-18T02:31:27Z

Nope, usually we don't have any release dates. Whenever something big gets in @keflavich does a release. 0.3.5 is very much due to get released as there were tons of fixes to esasky, and also we have the new gaia module. However the latter has some serious python deprecations, so I would like to see those fixed before the release.
So while I can't promise much, the prospects are that whenever this goes in, there can be a release almost instantaneously.

pllim · 2017-03-20T17:47:31Z

Can we ignore the "coverage decrease" warning? I believe it is misleading because the meat of my tests requires remote data.

Here is the test result for python setup.py -P vo_conesearch --remote-data:

===== test session starts =====
platform linux -- Python 3.5.2, pytest-3.0.4, py-1.4.31, pluggy-0.4.0

Running tests with astroquery version 0.3.5.dev3814.
Running tests in lib.linux-x86_64-3.5/astroquery/vo_conesearch docs/vo_conesearch.

Date: 2017-03-20T13:42:12

Platform: Linux-2.6.32-642.11.1.el6.x86_64-x86_64-with-redhat-6.8-Santiago

Executable: .../anaconda/envs/dadf/bin/python

Full Python Version: 
3.5.2 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:53:06) 
[GCC 4.4.7 20120313 (Red Hat 4.4.7-1)]

encodings: sys: utf-8, locale: UTF-8, filesystem: utf-8
byteorder: little
float info: dig: 15, mant_dig: 15

Numpy: 1.12.0
Matplotlib: 2.0.0
Astropy: 2.0.dev17886
APLpy: not available
pyregion: not available
Using Astropy options: remote_data: any.

rootdir: /tmp/astroquery-test-88_2a199, inifile: setup.cfg
collected 48 items 

astroquery/vo_conesearch/tests/test_conesearch.py ......x..........x....
astroquery/vo_conesearch/tests/test_vos_catalog.py ...............
astroquery/vo_conesearch/validator/tests/test_inpect.py ....
astroquery/vo_conesearch/validator/tests/test_validate.py ....
../docs/vo_conesearch/client.rst .
../docs/vo_conesearch/validator.rst .
../docs/vo_conesearch/vo_conesearch.rst .

===== pytest-warning summary =====
WC1 None [pytest] section in setup.cfg files is deprecated, use [tool:pytest] instead.
===== 46 passed, 2 xfailed, 1 pytest-warnings in 43.01 seconds =====

bsipocz · 2017-03-20T17:50:22Z

@pllim - Sure, don't worry about coverage when adding a new module.

pllim · 2017-03-20T18:57:36Z

FYI -- CI report is in the first (rebased and squashed) commit. I skipped CI for the second commit because it is only PEP8 fix and PEP8 test is allowed to fail anyway (I was just being OCD).

bsipocz · 2017-03-20T19:22:14Z

This will definitely need a changelog entry.
@keflavich - would you review it before 0.3.5?

pllim · 2017-03-20T19:24:44Z

@bsipocz , there is no "0.3.6" section in change log yet, but I'll be happy to add it when the change log has the relevant section (or if you decide to include this in 0.3.5). Please let me know.

bsipocz · 2017-03-20T19:33:30Z

OK, sure. Let's add this to 0.3.5.
Since there is one more critical issue to solve before the release, this will most probably be merged by then.

keflavich · 2017-03-20T19:33:31Z

This is somewhat overwhelming to review. @pllim can you suggest any particular sections to review that are specific to the astropy->astroquery migration? I assume much of this has undergone review when being incorporated into astropy?

MSeifert04 · 2017-03-20T19:41:07Z

@pllim One general comment: There are lots of :py:mod: and :py:class: sphinx directives in there. Is that on purpose?

pllim · 2017-03-20T19:42:28Z

Re: @keflavich

Oh, the original PR in Astropy (astropy/astropy#552) only had like 181 comments...

I would recommend that you start with the following order:

Introduction
Getting started
core.py, and __init__.py for the accompanying config.

While I implemented query_region for Astroquery, it is only for the sake of comformity to a common API but people who are used to Astropy behaviour will still use the non-conforming conesearch.py. Even so, it returns VO table (not the unified Astropy table); I decided against returning a different thing from what people are used to already. If this means that I have to list Cone Search under "does not follow common API", then let me know and I'll correct the doc.

Since Cone Search already has its own async interface that works, I also decided against query_region_async and instead have it raise NotImplementedError and point user to the old API.

In short: Focus on the Astroquery common API and high level stuff, and feel free to ignore the validator if you want. The low level stuff should stay as-is.

MSeifert04 · 2017-03-20T19:44:21Z

+    def __getattr__(self, what):
+        """Expose ``executor`` and ``future`` methods."""
+        try:
+            return getattr(self.executor, what)


Just throwing in some random stuff: getattr takes a third argument in case it encounters an AttributeError. However return getattr(self.executor, what, getattr(self.future, what)) might be hard to swallow 😄

Oh, that's good to know. And I agree with your second sentence. 😄

pllim · 2017-03-20T19:44:43Z

Re: @MSeifert04 -- Yes, it was on purpose (pretty hard to type those things by accident 😂 ) but it was also written a long time ago. Since Sphinx isn't complaining, I have no motivation to revisit them.

MSeifert04 · 2017-03-20T19:47:49Z

@pllim Just out of interest: Do you know why you did that on purpose?

bsipocz · 2017-03-20T19:48:58Z

@pllim - If you have added the most common common API functions/methods then there is no need to list it under the "does not follow..." section.
With the recently added gaia and this one, and maybe fix other more complex future additions, I think it's about time to restructure the index page a bit (though I don't have any firm idea what would be the best).

pllim · 2017-03-20T19:53:44Z

@bsipocz -- Thanks for the feedback. I have attached a change log entry in the second commit.

@MSeifert04 -- I am sure I knew why back in 2012 but now I have to guess... Maybe I was telling myself that "explicit is better than implicit" or maybe I just copied and pasted from someone...

bsipocz · 2017-03-20T19:59:58Z

+- TAP/TAP+ - added Table Access Protocol utility and the ESAC
  Science Data Centre (ESDC) extension.
+- VO Cone Search - added module to access Virtual Observatory's Simple
+  Cone Search. This is ported from ``astropy.vo``.


Could you add the PR number? We don't yet let astrobot loose on this repo, but I have high hopes it.

pllim · 2017-03-21T13:55:12Z

CIs passed. The failure was reported only by coverage, which we have agreed to ignore. FYI.

keflavich

I only found one small thing to comment on. I think this is ready to merge.

keflavich · 2017-03-21T14:04:51Z

+
+    def __init__(self):
+        if not self.URL.endswith(('?', '&')):
+            raise InvalidAccessURL("url should already end with '?' or '&'")


I don't think 'already' fits here.

Do you mean removing that word from the error message, or just not check it at all?

keflavich · 2017-03-21T14:19:07Z

Just that word

Also added change log entry and addressed feedback comments.

pllim · 2017-03-21T14:27:00Z

Okay, done. I rebased, squashed, and letting CIs run one more time.

bsipocz · 2017-03-21T17:07:17Z

Give @keflavich approved it I dared to hit the button.

Other things are sorted out for 0.3.5, too, so maybe it's release time? :)

pllim mentioned this pull request Mar 18, 2017

Remove astropy.vo sub-package astropy/astropy#5558

Closed

bsipocz added this to the 0.3.6 milestone Mar 18, 2017

bsipocz added enhancement New Service vo_conesearch labels Mar 18, 2017

pllim force-pushed the add-vo-conesearch branch from f9aa77c to 7734f8b Compare March 20, 2017 17:18

pllim force-pushed the add-vo-conesearch branch from 7734f8b to 61afb8b Compare March 20, 2017 17:51

bsipocz modified the milestones: 0.3.5, 0.3.6 Mar 20, 2017

MSeifert04 reviewed Mar 20, 2017

View reviewed changes

pllim force-pushed the add-vo-conesearch branch from 754831a to 013211f Compare March 20, 2017 19:48

bsipocz reviewed Mar 20, 2017

View reviewed changes

bsipocz mentioned this pull request Mar 20, 2017

PEP8 fixes #866

Merged

keflavich approved these changes Mar 21, 2017

View reviewed changes

Ported Cone Search over from Astropy. Added Astroquery-compliant API.

eaa6df9

Also added change log entry and addressed feedback comments.

pllim force-pushed the add-vo-conesearch branch from 107adfe to eaa6df9 Compare March 21, 2017 14:24

bsipocz merged commit a3f917f into astropy:master Mar 21, 2017

pllim deleted the add-vo-conesearch branch March 21, 2017 20:40

pllim mentioned this pull request Mar 23, 2017

Deprecate VO Simple Cone Search astropy/astropy#5904

Merged

Conversation

pllim commented Mar 18, 2017

Uh oh!

bsipocz commented Mar 18, 2017

Uh oh!

pllim commented Mar 18, 2017

Uh oh!

bsipocz commented Mar 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Mar 20, 2017

Uh oh!

pllim commented Mar 20, 2017

Uh oh!

bsipocz commented Mar 20, 2017

Uh oh!

pllim commented Mar 20, 2017

Uh oh!

bsipocz commented Mar 20, 2017

Uh oh!

keflavich commented Mar 20, 2017

Uh oh!

MSeifert04 commented Mar 20, 2017

Uh oh!

pllim commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

pllim Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

pllim commented Mar 20, 2017

Uh oh!

MSeifert04 commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Mar 20, 2017

Uh oh!

pllim commented Mar 20, 2017

Uh oh!

bsipocz Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

pllim Mar 20, 2017

Choose a reason for hiding this comment

Uh oh!

pllim commented Mar 21, 2017

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

keflavich Mar 21, 2017

Choose a reason for hiding this comment

Uh oh!

pllim Mar 21, 2017

Choose a reason for hiding this comment

Uh oh!

keflavich commented Mar 21, 2017 via email

Uh oh!

pllim commented Mar 21, 2017

Uh oh!

bsipocz commented Mar 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bsipocz commented Mar 18, 2017 •

edited

Loading

pllim commented Mar 20, 2017 •

edited

Loading

pllim commented Mar 20, 2017 •

edited

Loading

MSeifert04 commented Mar 20, 2017 •

edited

Loading