Esasky #758

imbasimba · 2016-09-29T09:04:05Z

First implementation of ESASky module

…gion in catalogs.

…ed readability

…mments, added basic tests

…ted minor errors in comments.

…_images

keflavich

I've left a series of mostly stylistic comments. The module is looking very nearly complete, though a few more tests would probably be helpful.

The most important changes for getting this merged into astroquery is to fix the issues related to testing, since right now the tests will not work properly.

This is looking great!

keflavich · 2016-10-04T04:38:18Z

astroquery/esasky/core.py

+        """
+        return self._json_object_field_to_list(self._get_catalogs_json(cache = True), self.__MISSION_STRING)    
+
+    def query_object_maps(self, position, missions = __ALL_STRING, get_query_payload = False, cache = True):


Minor style comments:

in function definitions, we prefer not to have spaces surrounding = signs.

when possible, try to wrap lines at <= 80 characters.

keflavich · 2016-10-04T04:39:36Z

astroquery/esasky/core.py

+        query_region_maps("265.05, 69.0", 14*u.arcmin, "Herschel")
+        query_region_maps("265.05, 69.0", ["Herschel", "HST"])
+        """        
+        sanatized_position = self._sanatize_input_position(position)


spelling: sanitize

keflavich · 2016-10-04T04:40:11Z

astroquery/esasky/core.py

+
+        self._store_query_result_maps(query_result, sanatized_missions, coordinates, sanatized_radius, get_query_payload, cache)
+
+        if(get_query_payload):


parentheses aren't necessary here, but they're acceptable. Please add a space between if and (

keflavich · 2016-10-04T04:41:37Z

astroquery/esasky/core.py

+                    maps[query_mission] = self._get_maps_for_mission(sanatized_query_table_list[query_mission], query_mission, download_folder, cache)
+                    break
+        if(len(sanatized_query_table_list) > 0):
+            print("Maps available at %s" %os.path.abspath(download_folder))


Use log.info instead of print here (it gives users finer-tuned control over the code's verbosity)

keflavich · 2016-10-04T04:42:23Z

astroquery/esasky/core.py

+
+        maps = dict()
+
+        map_query_result = self.query_region_maps(sanatized_position, sanatized_radius, sanatized_missions, False)


it would be helpful for readability to specify these as keywords, at least for the last parameter (what is being set to False here? Hard to tell without scrolling through the code)

keflavich · 2016-10-04T04:56:18Z

astroquery/esasky/tests/test_maps.py

@@ -0,0 +1,23 @@
+from astroquery.esasky import ESASky
+import astropy.units as u
+from astropy.coordinates import SkyCoord


this is a good interactive test but the tests below should all be put into some function def test_something so that it's not called at import time and is run when we run overall tests

keflavich · 2016-10-04T04:56:43Z

docs/esasky/esasky.rst

+a choice of catalogs or missions.
+
+Get the available catalog names
+-------------------


number of dashes should match number of characters in previous line

keflavich · 2016-10-04T04:57:28Z

docs/esasky/esasky.rst

+---------------
+
+There are two query objects methods in this module query_object_catalogs
+and query_object_maps. They both work in exactly the same way except 


surround function names in backticks (or maybe double backticks... I can never remember sphinx syntax exactly. One or the other will still do something neat....)

keflavich · 2016-10-04T04:57:46Z

docs/esasky/esasky.rst

+.. code-block:: python
+
+    >>> from astroquery.esasky import ESASky
+	>>> import astropy.units as u


spacing problem here (too many tabs/spaces)

also, you're not using units in this example

keflavich · 2016-10-04T04:58:35Z

docs/esasky/esasky.rst

+
+.. code-block:: python
+
+    >>> for table_name in result.keys():


.keys() is not needed here

…and explicit print for integral fetching

… now returns a HDUList

keflavich · 2016-10-06T21:16:26Z

To run tests locally, use python setup.py test -P esasky. It reveals a few failures I'll try to help correct.

keflavich · 2016-10-06T21:10:47Z

astroquery/esasky/tests/test_esasky.py

-# similarly fill in tests for each of the methods
-# look at tests in existing modules for more examples
-
+def test_esasky_query_region_maps_invalid_position(self):


since this is not in a class, self should not be given as an input parameter. I'm not really sure what's going on in this code (where is assertRaises defined?), but it causes the following exceptions:
https://travis-ci.org/astropy/astroquery/jobs/165599483#L1212

ah, this should be in a class that inherits from python's unittest.TestCase

Fixes to local tests

keflavich · 2016-10-07T16:01:22Z

astroquery/esasky/core.py

 import tempfile
 import tarfile
 import sys
+from __future__ import print_function


Thanks! I was just about to report this.

bsipocz · 2016-10-10T12:11:53Z

@imbasimba - This looks very nice, thank you for adding esasky to astroquery.

bsipocz · 2016-10-10T12:39:41Z

docs/esasky/esasky.rst

+Query an object
+---------------
+
+There are two query objects methods in this module :meth:`~query_object_catalogs`


A minor comment, just for information: Full path is needed to resolve these method names as links in the docs, e.g. ~astroquery.esasky.ESASkyClass.query_object_catalogs. There are quite a few other cases below, too.

However it's not important at all at the moment, and I plan to go through the whole of astroquery to fix these cases.

imbasimba · 2016-10-10T13:10:17Z

Great!
@keflavich @bsipocz - Is there anything else that needs to be done before we can merge this pull request?

bsipocz · 2016-10-10T14:11:40Z

@imbasimba - The coverage failure is a red herring, so this looks good to me as is. However I'll leave the final word and honour of merging to @keflavich as he did all the review.

keflavich · 2016-10-10T15:45:10Z

astroquery/esasky/tests/test_esasky_remote.py

+
+    def test_esasky_query_region_maps(self):
+        result = esasky.core.ESASkyClass().query_region_maps("M51", "5 arcmin")
+        assert isinstance(result, Table)


In these 4 tests, this should be isinstance(result, TableList) (which also requires that TableList be imported above)

keflavich · 2016-10-10T15:45:45Z

I found one more small bug when running the remote-tests. Fix that, then I'll merge.

Ivan V and others added 21 commits September 7, 2016 15:07

First implementation of ESASky queries by Ivan

a8d8449

Extracted methods and renamed variables to improve readability

5d754e0

Added some dynamic query building and added possibility to query a re…

1e4b845

…gion in catalogs.

Added query region for observations.

ebdc8b5

Added get maps feature

a14c2e8

Added get_maps for herschel, removed old code, refactored for increas…

1b02518

…ed readability

Refactored code, added comments and fixed minor bugs

1e19266

Removed old files

364914b

Added get_images() and query_object(), fixed minor bugs, corrected co…

144c015

…mments, added basic tests

Removed empty tables from return values of query methods

8d474b9

Added possibility to query for a list of missions or catalogs. Correc…

09a2cbf

…ted minor errors in comments.

Added mission parameter for get_maps and get_images

339aeb8

Added parameter for choosing the download folder for get_maps and get…

4c207f7

…_images

Removed fits files

fe18b58

Added minimum documentation for ESASky

0783f0a

Converted return values of queries to TableLists

610eb68

Added all metadata for maps and catalogs.

50fe4cf

Added documentation for query methods.

ec667f7

Restored get_query_payload functionality

ed9c211

Added checks for input

df0f7c4

Started using the caching system

99e32e4

keflavich reviewed Oct 4, 2016

View reviewed changes

imbasimba added 8 commits October 4, 2016 15:25

Fixed most review comments

b33a35b

Bugfix for query builder error

3b47a95

Fully implemented query_object

5697f5e

Minor bugfixes, changed catalog object query to a radis of 5 arcsec, …

28c24a9

…and explicit print for integral fetching

Minor bugfix for integral

9435799

Improved tests

60528dc

removed excessive structure for get_images and get_maps. Each mission…

068b343

… now returns a HDUList

Minor refactoring

bc82938

imbasimba added 2 commits October 6, 2016 15:32

Updated documentation

9ad8cb3

Moved from dev server to live

87c77ad

keflavich reviewed Oct 6, 2016

View reviewed changes

keflavich and others added 5 commits October 6, 2016 15:38

fix local tests

415eff0

add catalogs for test

54a1f3f

Minor bugfix for get_maps

c891abc

Merge pull request #2 from keflavich/ivvv-esasky2

c6af1d5

Fixes to local tests

Added import for python version 2

bd705c3

keflavich reviewed Oct 7, 2016

View reviewed changes

imbasimba added 4 commits October 7, 2016 18:06

Moved import to top of file

5b310be

Moved print function import to top of file

d3b824d

Updated indentations in comments

b13018e

Added esasky to toctree

3c4d4ab

bsipocz added New Service esasky labels Oct 10, 2016

bsipocz reviewed Oct 10, 2016

View reviewed changes

Fixed method resolve issuse in docs

ca3f74a

Fixed more method resolve issuse in docs

eb69743

keflavich reviewed Oct 10, 2016

View reviewed changes

Corrected remote tests

8abb111

keflavich merged commit d8ab59d into astropy:master Oct 10, 2016

bsipocz modified the milestone: 0.3.3 Nov 21, 2016


		self._store_query_result_maps(query_result, sanatized_missions, coordinates, sanatized_radius, get_query_payload, cache)

		if(get_query_payload):


		maps = dict()

		map_query_result = self.query_region_maps(sanatized_position, sanatized_radius, sanatized_missions, False)

Uh oh!

Esasky #758

Esasky #758

Uh oh!

Conversation

imbasimba commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keflavich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keflavich Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keflavich commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imbasimba commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keflavich commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imbasimba commented Sep 29, 2016 •

edited

Loading

keflavich Oct 4, 2016 •

edited

Loading

imbasimba commented Oct 10, 2016 •

edited

Loading