diff --git a/CHANGES.rst b/CHANGES.rst index c6158ea041..b84755815b 100644 --- a/CHANGES.rst +++ b/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 ^^^^ diff --git a/astroquery/heasarc/__init__.py b/astroquery/heasarc/__init__.py index 3aacd9c3cc..588136e1f3 100644 --- a/astroquery/heasarc/__init__.py +++ b/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', diff --git a/astroquery/heasarc/core.py b/astroquery/heasarc/core.py index 6605ad5f34..9a0094f0bc 100644 --- a/astroquery/heasarc/core.py +++ b/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 diff --git a/astroquery/heasarc/tests/test_heasarc.py b/astroquery/heasarc/tests/test_heasarc.py index 3831c9083d..632393040d 100644 --- a/astroquery/heasarc/tests/test_heasarc.py +++ b/astroquery/heasarc/tests/test_heasarc.py @@ -58,8 +58,8 @@ def __init__(self, desc, cols=[]): def search(self, query, language='ADQL', maxrec=1000, uploads=None): if "SELECT COUNT(*) FROM" in query: - return TAPResults(votable.from_table(Table([[3055]], names=['count'], - dtype=['int64']))) + return TAPResults(votable.from_table( + Table([[3055]], names=['count'], dtype=['int64']))) else: return MockResult() @@ -71,21 +71,22 @@ def to_table(self): @pytest.fixture def mock_tap(): - with patch('astroquery.heasarc.core.HeasarcClass.tap', new_callable=PropertyMock) as tap: + with patch('astroquery.heasarc.core.HeasarcClass.tap', + new_callable=PropertyMock) as tap: tap.return_value = MockTap() yield tap @pytest.fixture def mock_default_cols(): - with patch('astroquery.heasarc.core.HeasarcClass._get_default_columns') as get_cols: + with patch('astroquery.heasarc.core.HeasarcClass._get_default_columns') as get_cols: # noqa get_cols.return_value = ['col-3', 'col-2'] yield get_cols @pytest.fixture def mock_meta(): - with patch('astroquery.heasarc.core.HeasarcClass._meta', new_callable=PropertyMock) as meta: + with patch('astroquery.heasarc.core.HeasarcClass._meta', new_callable=PropertyMock) as meta: # noqa meta.return_value = Table(dict( table=['tab1', 'tab2', 'tab1', 'tab1'], par=['p1', 'p2', 'p3', ''], @@ -190,7 +191,8 @@ def test_query_region_polygon_no_unit(): (10.0, 10.1), (10.0, 10.0), ] - with pytest.warns(UserWarning, match="Polygon endpoints are being interpreted as"): + with pytest.warns(UserWarning, + match="Polygon endpoints are being interpreted as"): query = Heasarc.query_region( catalog="suzamaster", spatial="polygon", @@ -226,7 +228,8 @@ def test_query_allsky(): def test_spatial_invalid(spatial): with pytest.raises(ValueError): Heasarc.query_region( - OBJ_LIST[0], catalog="invalid_spatial", columns="*", spatial=spatial + OBJ_LIST[0], catalog="invalid_spatial", columns="*", + spatial=spatial ) @@ -239,13 +242,12 @@ def test_no_catalog(): with pytest.raises(InvalidQueryError): # OBJ_LIST[0] and radius added to avoid a remote call Heasarc.query_region( - OBJ_LIST[0], spatial="cone", columns="*", radius="2arcmin") + OBJ_LIST[0], catalog=None, spatial="cone") def test__query_execute_no_catalog(): with pytest.raises(InvalidQueryError): - # OBJ_LIST[0] and radius added to avoid a remote call - Heasarc._query_execute(None) + Heasarc._query_execute(catalog=None) def test_parse_constraints_no_filter(): @@ -254,7 +256,8 @@ def test_parse_constraints_no_filter(): def test_parse_constraints_range(): - constraints = Heasarc._parse_constraints(column_filters={"flux": (1e-12, 1e-10)}) + constraints = Heasarc._parse_constraints( + column_filters={"flux": (1e-12, 1e-10)}) assert constraints == ["flux BETWEEN 1e-12 AND 1e-10"] @@ -269,17 +272,20 @@ def test_parse_constraints_eq_str(): def test_parse_constraints_cmp_float(): - constraints = Heasarc._parse_constraints(column_filters={"flux": ('>', 1.2)}) + constraints = Heasarc._parse_constraints( + column_filters={"flux": ('>', 1.2)}) assert constraints == ["flux > 1.2"] def test_parse_constraints_cmp_float_2(): - constraints = Heasarc._parse_constraints(column_filters={"flux": ('>', 1.2), "magnitude": ('<=', 15)}) + constraints = Heasarc._parse_constraints( + column_filters={"flux": ('>', 1.2), "magnitude": ('<=', 15)}) assert constraints == ["flux > 1.2", "magnitude <= 15"] def test_parse_constraints_list(): - constraints = Heasarc._parse_constraints(column_filters={"flux": [1.2, 2.3, 3.4]}) + constraints = Heasarc._parse_constraints( + column_filters={"flux": [1.2, 2.3, 3.4]}) assert constraints == ["flux IN (1.2, 2.3, 3.4)"] @@ -302,7 +308,7 @@ def test_query_region_filter_range(): columns="*", get_query_payload=True, ) - assert query == "SELECT * FROM suzamaster WHERE flux BETWEEN 1e-12 AND 1e-10" + assert query == "SELECT * FROM suzamaster WHERE flux BETWEEN 1e-12 AND 1e-10" # noqa def test_query_region_filter_eq_float(): @@ -468,7 +474,7 @@ def test_meta_def(): # Use a new HeasarcClass object Heasarc = HeasarcClass() assert Heasarc._meta_info is None - with patch('astroquery.heasarc.core.HeasarcClass.query_tap') as mock_query_tap: + with patch('astroquery.heasarc.core.HeasarcClass.query_tap') as mock_query_tap: # noqa mock_query_tap.return_value = MockResult() meta = Heasarc._meta assert meta['value'][0] == 1.5 @@ -499,7 +505,8 @@ def test__list_catalogs(mock_tap): lab for lab in MockTap().tables.keys() if 'TAP' not in lab ] assert list(catalogs['description']) == [ - desc.description for lab, desc in MockTap().tables.items() if 'TAP' not in lab + desc.description for lab, desc in MockTap().tables.items() + if 'TAP' not in lab ] @@ -516,7 +523,8 @@ def test_list_catalogs_keywords_list_non_str(): def test__list_catalogs_keywords(mock_tap): catalogs = Heasarc.list_catalogs(keywords=['xmm']) assert list(catalogs['name']) == [ - lab for lab, desc in MockTap().tables.items() if 'TAP' not in lab and 'xmm' in desc.description.lower() + lab for lab, desc in MockTap().tables.items() + if 'TAP' not in lab and 'xmm' in desc.description.lower() ] @@ -537,7 +545,7 @@ def test_locate_data(): with pytest.raises( TypeError, match=( - "query_result need to be an astropy.table.Table or astropy.table.Row" + "query_result need to be an astropy.table.Table or astropy.table.Row" # noqa ) ): Heasarc.locate_data([1, 2]) @@ -575,7 +583,8 @@ def test__guess_host_sciserver(monkeypatch): assert Heasarc._guess_host(host=None) == 'sciserver' -@pytest.mark.parametrize("var", ["AWS_REGION", "AWS_REGION_DEFAULT", "AWS_ROLE_ARN"]) +@pytest.mark.parametrize( + "var", ["AWS_REGION", "AWS_REGION_DEFAULT", "AWS_ROLE_ARN"]) def test__guess_host_aws(monkeypatch, var): monkeypatch.setenv("AWS_REGION", var) assert Heasarc._guess_host(host=None) == 'aws' @@ -598,7 +607,7 @@ def test_download_data__missingcolumn(host): host_col = "access_url" if host == "heasarc" else host with pytest.raises( ValueError, - match=f"No {host_col} column found in the table. Call `~locate_data` first" + match=f"No {host_col} column found in the table. Call `~locate_data` first" # noqa ): Heasarc.download_data(Table({"id": [1]}), host=host) @@ -611,7 +620,8 @@ def test_download_data__sciserver(): with open(f'{datadir}/file.txt', 'w') as fp: fp.write('data') # include both a file and a directory - tab = Table({'sciserver': [f'{tmpdir}/data/file.txt', f'{tmpdir}/data']}) + tab = Table( + {'sciserver': [f'{tmpdir}/data/file.txt', f'{tmpdir}/data']}) # The patch is to avoid the test that we are on sciserver with patch('os.path.exists') as exists: exists.return_value = True @@ -639,12 +649,15 @@ def test_download_data__table_row(): with open(f'{datadir}/file.txt', 'w') as fp: fp.write('data') # include both a file and a directory - tab = Table({'sciserver': [f'{tmpdir}/data/file.txt', f'{tmpdir}/data']}) + tab = Table( + {'sciserver': [f'{tmpdir}/data/file.txt', f'{tmpdir}/data']}) # The patch is to avoid the test that we are on sciserver with patch('os.path.exists') as exists: exists.return_value = True - Heasarc.download_data(tab[0], host="sciserver", location=downloaddir) - Heasarc.download_data(tab[1], host="sciserver", location=downloaddir) + Heasarc.download_data( + tab[0], host="sciserver", location=downloaddir) + Heasarc.download_data( + tab[1], host="sciserver", location=downloaddir) assert os.path.exists(f'{downloaddir}/file.txt') assert os.path.exists(f'{downloaddir}/data') assert os.path.exists(f'{downloaddir}/data/file.txt') diff --git a/astroquery/heasarc/tests/test_heasarc_remote.py b/astroquery/heasarc/tests/test_heasarc_remote.py index 04d4dc35d7..f127d89182 100644 --- a/astroquery/heasarc/tests/test_heasarc_remote.py +++ b/astroquery/heasarc/tests/test_heasarc_remote.py @@ -10,8 +10,6 @@ from astropy.utils.exceptions import AstropyDeprecationWarning from astroquery.exceptions import NoResultsWarning -from pyvo.dal.exceptions import DALOverflowWarning - from astroquery.heasarc import Heasarc @@ -56,9 +54,6 @@ ], ] -# The MAXREC related overflow message is different in pyvo 1.7+, remove workaround when we have it as a minimum -overflow_message = r"Partial result set. Potential causes MAXREC|Result set limited by user- or server-supplied MAXREC" - @pytest.mark.remote_data class TestHeasarc: @@ -144,10 +139,12 @@ def test_list_catalogs__master(self): assert "swiftmastr" in catalogs def test_list_catalogs__keywords(self): - catalogs = list(Heasarc.list_catalogs(keywords="nustar", master=True)["name"]) + catalogs = list( + Heasarc.list_catalogs(keywords="nustar", master=True)["name"]) assert len(catalogs) == 1 and "numaster" in catalogs - catalogs = list(Heasarc.list_catalogs(keywords="xmm", master=True)["name"]) + catalogs = list( + Heasarc.list_catalogs(keywords="xmm", master=True)["name"]) assert len(catalogs) == 1 and "xmmmaster" in catalogs catalogs = list(Heasarc.list_catalogs(keywords=["swift", "rosat"], @@ -156,13 +153,6 @@ def test_list_catalogs__keywords(self): assert "rosmaster" in catalogs assert "rassmaster" in catalogs - def test_tap__maxrec(self): - query = "SELECT TOP 10 ra,dec FROM xray" - with pytest.warns(expected_warning=DALOverflowWarning, match=overflow_message): - result = Heasarc.query_tap(query=query, maxrec=5) - assert len(result) == 5 - assert result.to_table().colnames == ["ra", "dec"] - @pytest.mark.parametrize("tdefault", DEFAULT_COLS) def test__get_default_columns(self, tdefault): catalog, tdef = tdefault @@ -206,9 +196,12 @@ def test_download_data__heasarc_folder(self): with tempfile.TemporaryDirectory() as tmpdir: Heasarc.download_data(tab, host="heasarc", location=tmpdir) assert os.path.exists(f"{tmpdir}/stdprod") - assert os.path.exists(f"{tmpdir}/stdprod/FHed_1791a7b9-1791a931.gz") - assert os.path.exists(f"{tmpdir}/stdprod/FHee_1791a7b9-1791a92f.gz") - assert os.path.exists(f"{tmpdir}/stdprod/FHef_1791a7b9-1791a92f.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHed_1791a7b9-1791a931.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHee_1791a7b9-1791a92f.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHef_1791a7b9-1791a92f.gz") def test_download_data__s3_file(self): filename = "00README" @@ -234,9 +227,12 @@ def test_download_data__s3_folder(self, slash): Heasarc.enable_cloud(provider='aws', profile=None) Heasarc.download_data(tab, host="aws", location=tmpdir) assert os.path.exists(f"{tmpdir}/stdprod") - assert os.path.exists(f"{tmpdir}/stdprod/FHed_1791a7b9-1791a931.gz") - assert os.path.exists(f"{tmpdir}/stdprod/FHee_1791a7b9-1791a92f.gz") - assert os.path.exists(f"{tmpdir}/stdprod/FHef_1791a7b9-1791a92f.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHed_1791a7b9-1791a931.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHee_1791a7b9-1791a92f.gz") + assert os.path.exists( + f"{tmpdir}/stdprod/FHef_1791a7b9-1791a92f.gz") def test_query_mission_columns(self): with pytest.warns(AstropyDeprecationWarning): @@ -280,6 +276,18 @@ def test_row_count(self): cat = "suzamaster" assert Heasarc.count_rows(cat) == 3055 + def test_query_region_offset_with_no_column(self): + # use columns='*' to avoid remote call to obtain the default columns + _ = Heasarc.query_region( + OBJ_LIST[0], + catalog="suzamaster", + spatial="cone", + radius="2arcmin", + columns=None, + get_query_payload=True, + add_offset=True, + ) + @pytest.mark.remote_data class TestHeasarcBrowse: