Refactor astroquery.heasarc to use VO protocols #2997

zoghbi-a · 2024-04-25T10:43:45Z

This a major refactor of the heasarc module to use the VO interface to the archive. The main motivation is:

Allow for complex region and table queries.
Expose the TAP service.
Cleanup the tests.
Since the VO interface is the main archive interface, the archive will be able to support this module more.

The main changes inlcude:

The old class has been renamed HeasarcClass -> HeasarcBrowseClass. The initialized instance is also rename Heasarc -> HeasarcBrowse. The same for the test files.
The new HeasarcClass class uses an interface similar to those used in other modules e.g. ipac.irsa.
A deprecation message has been added to the methods used for querying the tables and columns.
Added the ability to download data from the main heasarc servers, Sciserver and from the cloud.

pep8speaks · 2024-04-25T10:43:54Z

Hello @zoghbi-a! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file astroquery/heasarc/core.py:

Line 99:13: W503 line break before binary operator
Line 120:13: W503 line break before binary operator
Line 486:17: W503 line break before binary operator
Line 506:13: W503 line break before binary operator

Comment last updated at 2024-05-15 14:58:22 UTC

bsipocz · 2024-05-10T01:35:25Z

The old class has been renamed HeasarcClass -> HeasarcBrowseClass. The initialized instance is also renamed Heasarc -> HeasarcBrowse. The same for the test files.

What is the motivation for this? Are there any datasets that are only accessible using the webform, but not the VO backends?

(If the only motivation is to keep what has been here, then it's not necessary, removing everything as part of the refactor is totally fine. Ideally, the old user codes should keep working, but with such a large backend restructure we also have precedence for breaking those)

Doing a proper review may take me until I'm back from the interop.

zoghbi-a · 2024-05-10T12:43:21Z

All the tables should available through the new API, so it is kept in case some people are using it. If it is ok to remove the old class, I don't see a stopper.

bsipocz · 2024-05-10T18:21:51Z

so it is kept in case some people are using it

A rename doesn't really solve this scenario, as the continued support would have only worked if the name was kept the same, maybe with a deprecation warning.

So, before diving into a review, I would suggest cleaning up the old class. Maybe try to keep as much of the test examples/docs examples working as possible, or maybe working with a deprecation warning (e.g. in case some of the keywords need to be dropped, or renamed).

zoghbi-a · 2024-05-10T21:53:53Z

The new class implements most of the useful methods of the old class with a deprecation warning. I will then delete the old class and keep the methods and warnings in the new class.

bsipocz

I only leave a WIP review for now, that focuses only on the API, and will try to come back and look into the code itself later, hopefully later this week or next week. I was thinking it's useful to leave what I have for now, than hold it back until I have the time to do the full review.

bsipocz · 2024-06-11T06:16:31Z

astroquery/heasarc/__init__.py

@@ -9,6 +9,9 @@
 The initial version of this was coded in a sprint at the
 "Python in astronomy" workshop in April 2015 by Jean-Christophe Leyder,
 Abigail Stevens, Antonio Martin-Carrillo and Christoph Deil.
+
+Updates to use the XAMIN service was added by Abdu Zoghbi


All of this and the above author 3 lines can be cleaned-up, too. Git carries the history, and we'll also work on a team website that will list whoever is actively maintain the module.

bsipocz · 2024-06-11T06:16:56Z

CHANGES.rst

+heasarc
+^^^^^^^
+
+- Refactor heasarc to use the VO backend. The old Heasarc class is now HeasarcBrowser [#2997]


Drop the second sentence as we don't keep the old class

bsipocz · 2024-06-11T06:19:48Z

astroquery/heasarc/core.py

-        return Table.read(*args, **kwargs)
-    else:
-        return Table.read(*args, **kwargs, unit_parse_strict='silent')
-

 @async_to_sync
 class HeasarcClass(BaseQuery):


Do inherit from the VO baseclass, too, using the order in the other modules (so e.g. the session is properly get passed back to pyvo, and a couple of other small things)

bsipocz · 2024-06-11T06:22:46Z

astroquery/heasarc/core.py

+        return self._tap
+
+    @property
+    def _meta(self):


This is a great idea, and great that the server returns the info in the table and not in the metadata of the table, as e.g astropy has serious toubles with parsing that non-column metadata

bsipocz · 2024-06-11T06:23:40Z

astroquery/heasarc/core.py

+        defaults = meta['par']
+        return defaults
+
+    def get_default_radius(self, table_name):


make this private, too, similar to _get_default_cols

bsipocz · 2024-06-11T06:42:23Z

astroquery/heasarc/core.py


-        return self.query_async(request_payload, cache=cache)
+    def get_links(self, query_result=None, tablename=None):


are these datalinks? Then call it get_datalinks instead (like in gaia), though here were are inconsistent yet again as alma has datalink instead. But not many modules have datalinks yet, so we can sort out the API now and use that for the future (and then clean up the inconsistent, but existing modules)

bsipocz · 2024-06-11T06:43:36Z

astroquery/heasarc/core.py

+
+        return dl_result
+
+    def enable_cloud(self, provider='aws', profile=None):


hopefully everthing cloud related will make its way up to pyvo, eventually, but until then it all can stay of course.

bsipocz · 2024-06-11T06:45:28Z

astroquery/heasarc/core.py


-        fits_content = fits.open(BytesIO(content))
+    def download_data(self, links, host='heasarc', location='.'):


I wonder how much of this can be put in the baseclass

bsipocz · 2024-06-11T06:47:09Z

astroquery/heasarc/core.py

+            The result from get_links
+        host : str
+            The data host. The options are: heasarc (defaul), sciserver, aws.
+            If host == 'sciserver', data is copied from the local mounted


this is very specific corner case only for sciserver. Will it work for smce, too?

bsipocz · 2024-06-11T06:49:12Z

astroquery/heasarc/tests/setup_package.py

-
-def get_package_data():
-    paths = [
-        os.path.join("data", "*.dat"),


I don't see the datafiles being deleted. Should they be?

zoghbi-a force-pushed the heasarc-refactor branch 2 times, most recently from 43e9fe4 to 7f74228 Compare April 25, 2024 20:42

zoghbi-a marked this pull request as ready for review April 25, 2024 20:52

bsipocz added the heasarc label May 10, 2024

bsipocz added this to the v0.4.8 milestone May 10, 2024

zoghbi-a force-pushed the heasarc-refactor branch from a746966 to 354384f Compare May 13, 2024 17:06

zoghbi-a and others added 9 commits May 15, 2024 10:57

added HeasarcXaminClass and some tests

450cb2b

started docs and changed core.py accordingly

d24539b

expand the download_data functionality

1764a91

moved xamin class to a separate file, with many updates and tests

feb15e7

removed build-docs folder which was added by mistake

6e1de65

moved Heasarc->HeasarcBrowse, Xamin->Heasarc for main code and tests

915ea98

update documentation for xamin

5faf0b4

changes to fix the CI

c461908

deprecate HeasarcBrowse

baf95ed

zoghbi-a force-pushed the heasarc-refactor branch from 354384f to baf95ed Compare May 15, 2024 14:58

nkphysics mentioned this pull request May 27, 2024

Vectorize HEASARC object queries #3013

Open

bsipocz requested changes Jun 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor astroquery.heasarc to use VO protocols #2997

Refactor astroquery.heasarc to use VO protocols #2997

zoghbi-a commented Apr 25, 2024 •

edited

Loading

pep8speaks commented Apr 25, 2024 •

edited

Loading

bsipocz commented May 10, 2024

zoghbi-a commented May 10, 2024

bsipocz commented May 10, 2024

zoghbi-a commented May 10, 2024

bsipocz left a comment

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024

bsipocz Jun 11, 2024


		return self.query_async(request_payload, cache=cache)
		def get_links(self, query_result=None, tablename=None):


		return dl_result

		def enable_cloud(self, provider='aws', profile=None):


		fits_content = fits.open(BytesIO(content))
		def download_data(self, links, host='heasarc', location='.'):

Refactor astroquery.heasarc to use VO protocols #2997

Are you sure you want to change the base?

Refactor astroquery.heasarc to use VO protocols #2997

Conversation

zoghbi-a commented Apr 25, 2024 • edited Loading

pep8speaks commented Apr 25, 2024 • edited Loading

Comment last updated at 2024-05-15 14:58:22 UTC

bsipocz commented May 10, 2024

zoghbi-a commented May 10, 2024

bsipocz commented May 10, 2024

zoghbi-a commented May 10, 2024

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoghbi-a commented Apr 25, 2024 •

edited

Loading

pep8speaks commented Apr 25, 2024 •

edited

Loading