Fix two bugs in calling heasarc.query_region

CHANGES.rst

@@ -110,6 +110,8 @@ heasarc
 - Add support for uploading tables when using TAP directly through ``query_tap``. [#3403]
 - Add automatic guessing for the data host in ``download_data``. [#3403]
 - Include method to count the number of rows in a specified table. [#3549]
+- Fix ``query_region`` for catalog=None. It should fail early. [#3630]
+- Fix ``query_region`` when passing ``add_offset`` along with ``columns=None``. [#3630]
 
 gaia
 ^^^^

astroquery/heasarc/__init__.py

@@ -40,7 +40,7 @@ class Conf(_config.ConfigNamespace):
 
 conf = Conf()
 
-from .core import Heasarc, HeasarcClass
+from .core import Heasarc, HeasarcClass  # noqa E402
 
 __all__ = ['Heasarc', 'HeasarcClass',
            'Conf', 'conf',

astroquery/heasarc/core.py

@@ -21,9 +21,8 @@
 
 
 class HeasarcClass(BaseVOQuery, BaseQuery):
-    """Class for accessing HEASARC data with VO protocol using the Xamin backend.
-
-
+    """Class for accessing HEASARC data with VO protocol using
+    the Xamin backend.
     """
 
     # we can move url to Config later
@@ -38,6 +37,11 @@ def __init__(self):
         self._tap = None
         self._datalink = None
         self._meta_info = None
+        self._catalog_msg = (
+            "catalog name is required! "
+            "Use 'xray' to search the master X-ray catalog. Or list catalogs "
+            "by calling :meth:`~astroquery.heasarc.HeasarcClass.list_catalogs`"
+        )
 
     @property
     def tap(self):
@@ -56,8 +60,9 @@ def _meta(self):
         This is a table that holds useful information such as
         the list of default columns per catalog, the reasonable default
         search radius per table that is appropriate for a mission etc.
-        Instead of making a server call for each catalog for that type information,
-        we do a single one and then post-process the resulting table.
+        Instead of making a server call for each catalog for that type
+        information, we do a single one and then post-process the resulting
+        table.
 
         These are not meant to be used directly by the user.
         """
@@ -291,6 +296,7 @@ def query_tap(self, query, *, maxrec=None, uploads=None):
             query, language='ADQL', maxrec=maxrec, uploads=uploads)
 
     def _query_execute(self, catalog=None, where=None, *,
+                       offset_column=None,
                        get_query_payload=False, columns=None,
                        verbose=False, maxrec=None):
         """Queries some catalog using the HEASARC TAP server based on the
@@ -304,6 +310,9 @@ def _query_execute(self, catalog=None, where=None, *,
         where : str
             The WHERE condition to be used in the query. It must
             include the 'WHERE' keyword or be empty.
+        offset_column: str or None
+            If add_offset is True in query_regoni, this contains the
+            the string that addes that to the query.
         get_query_payload : bool, optional
             If `True` then returns the generated ADQL query as str.
             Defaults to `False`.
@@ -326,8 +335,7 @@ def _query_execute(self, catalog=None, where=None, *,
             commons.suppress_vo_warnings()
 
         if catalog is None:
-            raise InvalidQueryError("catalog name is required! Use 'xray' "
-                                    "to search the master X-ray catalog")
+            raise InvalidQueryError(self._catalog_msg)
 
         if where is None:
             where = ''
@@ -343,6 +351,9 @@ def _query_execute(self, catalog=None, where=None, *,
         if columns is None:
             columns = ', '.join(self._get_default_columns(catalog))
 
+        if offset_column is not None:
+            columns += offset_column
+
         if '__row' not in columns and columns != '*':
             columns += ', __row'
 
@@ -422,8 +433,9 @@ def _parse_constraints(self, column_filters):
                        True, True, True, False)
     )
     def query_region(self, position=None, catalog=None, radius=None, *,
-                     spatial='cone', width=None, polygon=None, column_filters=None,
-                     add_offset=False, get_query_payload=False, columns=None, cache=False,
+                     spatial='cone', width=None, polygon=None,
+                     column_filters=None, add_offset=False,
+                     get_query_payload=False, columns=None, cache=False,
                      verbose=False, maxrec=None,
                      **kwargs):
         """Queries the HEASARC TAP server around a coordinate and returns a
@@ -479,7 +491,8 @@ def query_region(self, position=None, catalog=None, radius=None, *,
         add_offset: bool
             If True and spatial=='cone', add a search_offset column that
             indicates the separation (in arcmin) between the requested
-            coordinate and the entry coordinates in the catalog. Default is False.
+            coordinate and the entry coordinates in the catalog.
+            Default is False.
         get_query_payload : bool, optional
             If `True` then returns the generated ADQL query as str.
             Defaults to `False`.
@@ -502,6 +515,17 @@ def query_region(self, position=None, catalog=None, radius=None, *,
         if position is None and column_filters is not None:
             spatial = 'all-sky'
 
+        # check for a valid catalog name
+        if catalog is None:
+            raise InvalidQueryError(self._catalog_msg)
+
+        # add_offset is valid only with cone searches
+        if spatial != 'cone' and add_offset:
+            raise InvalidQueryError("add_offset is valid only spatial=='cone'")
+
+        # to hold the offset columns, if needed
+        offset_column = None
+
         if spatial.lower() == 'all-sky':
             where = ''
         elif spatial.lower() == 'polygon':
@@ -543,8 +567,10 @@ def query_region(self, position=None, catalog=None, radius=None, *,
                          f"'ICRS',{ra},{dec},{radius.to(u.deg).value}))=1")
                 # add search_offset for the case of cone
                 if add_offset:
-                    columns += (",DISTANCE(POINT('ICRS',ra,dec), "
-                                f"POINT('ICRS',{ra},{dec})) as search_offset")
+                    offset_column = (
+                        ",DISTANCE(POINT('ICRS',ra,dec), "
+                        f"POINT('ICRS',{ra},{dec})) as search_offset"
+                    )
             elif spatial.lower() == 'box':
                 if isinstance(width, str):
                     width = coordinates.Angle(width)
@@ -567,6 +593,7 @@ def query_region(self, position=None, catalog=None, radius=None, *,
 
         table_or_query = self._query_execute(
             catalog=catalog, where=where,
+            offset_column=offset_column,
             get_query_payload=get_query_payload,
             columns=columns, verbose=verbose,
             maxrec=maxrec
@@ -657,8 +684,9 @@ def locate_data(self, query_result=None, catalog_name=None):
 
         if catalog_name is None:
             if not hasattr(self, '_last_catalog_name'):
-                raise ValueError('locate_data needs a catalog_name, and none '
-                                 'found from a previous search. Please provide one.')
+                raise ValueError(
+                    'locate_data needs a catalog_name, and none '
+                    'found from a previous search. Please provide one.')
             catalog_name = self._last_catalog_name
         if not (
             isinstance(catalog_name, str)
@@ -687,7 +715,8 @@ def locate_data(self, query_result=None, catalog_name=None):
             dl_result['error_message'] != '',
             shrink=False
         )]
-        dl_result = dl_result[['ID', 'access_url', 'content_length', 'error_message']]
+        dl_result = dl_result[
+            ['ID', 'access_url', 'content_length', 'error_message']]
 
         # add sciserver and s3 columns
         newcol = [
@@ -785,9 +814,11 @@ def download_data(self, links, *, host=None, location='.'):
         Parameters
         ----------
         links : `astropy.table.Table` or `astropy.table.Row`
-            A table (or row) of data links, typically the result of locate_data.
+            A table (or row) of data links, typically the result of
+            locate_data.
         host : str or None
-            The data host. The options are: None (default), heasarc, sciserver, aws.
+            The data host. The options are: None (default), heasarc,
+            sciserver, aws.
             If None, the host is guessed based on the environment.
             If host == 'sciserver', data is copied from the local mounted
             data drive.
@@ -902,7 +933,7 @@ def _copy_sciserver(self, links, location='.'):
         Users should be using `~self.download_data` instead
 
         """
-        if not os.path.exists('/FTP/'):
+        if not os.path.exists('/FTP/nusatr'):
             raise FileNotFoundError(
                 'No data archive found. This should be run on Sciserver '
                 'with the data drive mounted.'
@@ -965,8 +996,8 @@ def count_rows(self, catalog):
         Parameters
         ----------
         catalog : str
-            The name of the catalog to query for a total number of rows. To list
-            the available catalogs, use
+            The name of the catalog to query for a total number of rows.
+            To list the available catalogs, use
             :meth:`~astroquery.heasarc.HeasarcClass.list_catalogs`.
 
         Returns