Merge pull request #2637 from keflavich/simbad_wrong_test_coordinates

Bugfix: SIMBAD test cleanup, ROW_LIMIT support

Author: bsipocz

CHANGES.rst

@@ -158,6 +158,11 @@ simbad
   radius as a string in ``query_region()`` and ``query_region_async()``.
   [#2494]
 - Optional keyword arguments are now keyword only. [#2609]
+- ``ROW_LIMIT`` is now respected when running region queries; previously, it
+  was ignored for region queries but respected for all others.  A new warning,
+  ``BlankResponseWarning``, is introduced for use when one or more query terms result
+  in a blank or missing row; previously, only a generic warning was issued.
+  [#2637]
 
 skyview
 ^^^^^^^

astroquery/exceptions.py

@@ -8,7 +8,8 @@
 __all__ = ['TimeoutError', 'InvalidQueryError', 'RemoteServiceError',
            'TableParseError', 'LoginError', 'ResolverError',
            'NoResultsWarning', 'LargeQueryWarning', 'InputWarning',
-           'AuthenticationWarning', 'MaxResultsWarning', 'CorruptDataWarning']
+           'AuthenticationWarning', 'MaxResultsWarning', 'CorruptDataWarning',
+           'BlankResponseWarning']
 
 
 class TimeoutError(Exception):
@@ -111,3 +112,10 @@ class EmptyResponseError(ValueError):
     Astroquery error class to be raised when the query returns an empty result
     """
     pass
+
+
+class BlankResponseWarning(AstropyWarning):
+    """
+    Astroquery warning to be raised if one or more rows in a table are bad, but
+    not all rows are.
+    """

astroquery/simbad/core.py

@@ -19,7 +19,7 @@
 
 from astroquery.query import BaseQuery
 from astroquery.utils import commons, async_to_sync
-from astroquery.exceptions import TableParseError, LargeQueryWarning
+from astroquery.exceptions import TableParseError, LargeQueryWarning, BlankResponseWarning
 from . import conf
 
 
@@ -135,7 +135,9 @@ def __warn(self):
             warnings.warn("Warning: The script line number %i raised "
                           "an error (recorded in the `errors` attribute "
                           "of the result table): %s" %
-                          (error.line, error.msg))
+                          (error.line, error.msg),
+                          BlankResponseWarning
+                          )
 
     def __get_section(self, section_name):
         if section_name in self.__indexes:
@@ -938,7 +940,8 @@ def _get_query_header(self, get_raw=False):
         # if get_raw is set then don't fetch as votable
         if get_raw:
             return ""
-        return "votable {" + ','.join(self.get_votable_fields()) + "}\nvotable open"
+        row_limit = f"set limit {self.ROW_LIMIT}\n" if self.ROW_LIMIT > 0 else ""
+        return f"{row_limit}votable {{{','.join(self.get_votable_fields())}}}\nvotable open"
 
     def _get_query_footer(self, get_raw=False):
         return "" if get_raw else "votable close"
@@ -960,8 +963,6 @@ def _args_to_payload(self, *args, **kwargs):
         votable_header = self._get_query_header(get_raw)
         votable_footer = self._get_query_footer(get_raw)
 
-        if self.ROW_LIMIT > 0:
-            script = "set limit " + str(self.ROW_LIMIT)
         script = "\n".join([script, votable_header, command])
         using_wildcard = False
         if kwargs.get('wildcard'):

astroquery/simbad/tests/test_simbad_remote.py

@@ -10,10 +10,11 @@
 from astroquery.simbad import Simbad
 # Maybe we need to expose SimbadVOTableResult to be in the public API?
 from astroquery.simbad.core import SimbadVOTableResult
+from astroquery.exceptions import BlankResponseWarning
 
 
 # M42 coordinates
-ICRS_COORDS_M42 = SkyCoord("05h35m17.3s -05h23m28s", frame='icrs')
+ICRS_COORDS_M42 = SkyCoord("05h35m17.3s -05d23m28s", frame='icrs')
 ICRS_COORDS_SgrB2 = SkyCoord(266.835*u.deg, -28.38528*u.deg, frame='icrs')
 multicoords = SkyCoord([ICRS_COORDS_M42, ICRS_COORDS_SgrB2])
 
@@ -99,8 +100,7 @@ def test_query_catalog(self, temp_dir):
 
     def test_query_region_async(self, temp_dir):
         simbad = Simbad()
-        # TODO: rewise once ROW_LIMIT is working
-        simbad.TIMEOUT = 100
+        simbad.ROW_LIMIT = 100
         simbad.cache_location = temp_dir
         response = simbad.query_region_async(
             ICRS_COORDS_M42, radius=2 * u.deg, equinox=2000.0, epoch='J2000')
@@ -112,12 +112,12 @@ def test_query_region_async_vector(self, temp_dir, radius):
         simbad = Simbad()
         simbad.cache_location = temp_dir
         response1 = simbad.query_region_async(multicoords, radius=radius)
-        assert response1.request.body == 'script=votable+%7Bmain_id%2Ccoordinates%7D%0Avotable+open%0Aquery+coo+5%3A35%3A17.3+-80%3A52%3A00+radius%3D0.5s+frame%3DICRS+equi%3D2000.0%0Aquery+coo+17%3A47%3A20.4+-28%3A23%3A07.008+radius%3D0.5s+frame%3DICRS+equi%3D2000.0%0Avotable+close'   # noqa
+        assert response1.request.body == 'script=votable+%7Bmain_id%2Ccoordinates%7D%0Avotable+open%0Aquery+coo+5%3A35%3A17.3+-5%3A23%3A28+radius%3D0.5s+frame%3DICRS+equi%3D2000.0%0Aquery+coo+17%3A47%3A20.4+-28%3A23%3A07.008+radius%3D0.5s+frame%3DICRS+equi%3D2000.0%0Avotable+close'   # noqa
 
     def test_query_region(self, temp_dir):
         simbad = Simbad()
-        # TODO: rewise once ROW_LIMIT is working
         simbad.TIMEOUT = 100
+        simbad.ROW_LIMIT = 100
         simbad.cache_location = temp_dir
         result = simbad.query_region(ICRS_COORDS_M42, radius=2 * u.deg,
                                      equinox=2000.0, epoch='J2000')
@@ -149,7 +149,8 @@ def test_query_multi_object(self, temp_dir):
         assert len(result) == 2
         assert len(result.errors) == 0
 
-        result = simbad.query_objects(['M32', 'M81', 'gHer'])
+        with pytest.warns(BlankResponseWarning):
+            result = simbad.query_objects(['M32', 'M81', 'gHer'])
         # 'gHer' is not a valid Simbad identifier - it should be 'g Her' to
         # get the star
         assert len(result) == 2
@@ -182,29 +183,25 @@ def test_query_object_ids(self, temp_dir):
     def test_null_response(self, temp_dir, function):
         simbad = Simbad()
         simbad.cache_location = temp_dir
-        assert (simbad.__getattribute__(function)('idonotexist')
-                is None)
+        with pytest.warns(BlankResponseWarning):
+            assert (simbad.__getattribute__(function)('idonotexist')
+                    is None)
 
     # Special case of null test: list of nonexistent parameters
     def test_query_objects_null(self, temp_dir):
         simbad = Simbad()
         simbad.cache_location = temp_dir
-        assert simbad.query_objects(['idonotexist', 'idonotexisteither']) is None
+        with pytest.warns(BlankResponseWarning):
+            assert simbad.query_objects(['idonotexist', 'idonotexisteither']) is None
 
-    # Special case of null test: zero-sized region
-    def test_query_region_null(self, temp_dir):
+    # Special case of null test: zero-size and very small region
+    @pytest.mark.parametrize('radius', ["0d", 1.0*u.marcsec])
+    def test_query_region_null(self, temp_dir, radius):
         simbad = Simbad()
         simbad.cache_location = temp_dir
-        result = simbad.query_region(SkyCoord("00h01m0.0s 00h00m0.0s"), radius="0d",
-                                     equinox=2000.0, epoch='J2000')
-        assert result is None
-
-    # Special case of null test: very small region
-    def test_query_small_region_null(self, temp_dir):
-        simbad = Simbad()
-        simbad.cache_location = temp_dir
-        result = simbad.query_region(SkyCoord("00h01m0.0s 00h00m0.0s"), radius=1.0 * u.marcsec,
-                                     equinox=2000.0, epoch='J2000')
+        with pytest.warns(BlankResponseWarning):
+            result = simbad.query_region(SkyCoord("00h01m0.0s 00h00m0.0s"), radius=1.0 * u.marcsec,
+                                         equinox=2000.0, epoch='J2000')
         assert result is None
 
     # Special case : zero-sized region with one object