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 old HeasarcClass has been removed, and its main methods are included in the class with a deprecation message.
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 98:13: W503 line break before binary operator
Line 119:13: W503 line break before binary operator
Line 483:17: W503 line break before binary operator
Line 503:13: W503 line break before binary operator

Comment last updated at 2025-01-10 21:26:57 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.

astroquery/heasarc/__init__.py

CHANGES.rst

astroquery/heasarc/core.py

astroquery/heasarc/tests/setup_package.py

zoghbi-a · 2024-10-17T15:48:18Z

@bsipocz, is there a timeline for completing the review?

codecov · 2024-10-17T19:04:40Z

Codecov Report

Attention: Patch coverage is 65.88235% with 87 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@23d3471). Learn more about missing BASE report.
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
astroquery/heasarc/core.py	65.47%	87 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2997   +/-   ##
=======================================
  Coverage        ?   67.60%           
=======================================
  Files           ?      229           
  Lines           ?    18473           
  Branches        ?        0           
=======================================
  Hits            ?    12489           
  Misses          ?     5984           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bsipocz

I did an airport review, thus couldn't run the remote tests and their coverage.
Neither was building the documentation, but what I have noticed that it is full of both code and text typos, so please have a careful look at it.

The module itself looks reasonably good, my primary comments are the same I already left in the summer, namely it would be great if methods and arg names could be more similar to those already existing in some of the modules (e.g. avoid using table as a kwarg as it leads to either confusion or code mistakes.

astroquery/heasarc/core.py

bsipocz · 2024-10-18T21:16:31Z

astroquery/heasarc/core.py

+    VO_URL = conf.VO_URL
+    TAR_URL = conf.TAR_URL
+    S3_BUCKET = conf.S3_BUCKET


Do we need these here, or the properties could pick them up? (like we do in alma or the other modules that use pyvo based tap?)

Are you suggesting that these should not be configurable but rather as fixed variables?

I am inclined to leave it like this as it helps with testing staging and test servers.

docs/heasarc/heasarc.rst

zoghbi-a · 2025-01-09T19:01:19Z

1ce3523: rebasing to pick the latest changes in main and squashing all the changes.

bsipocz

There are a lot of minor comments mostly focusing on consistency, and fixing a couple of typos in the docs, too. Please try to clean them up.

More importantly, I see 26 remote tests and doctest failures, those should be fixed before merging.

bsipocz · 2025-01-11T05:34:03Z

astroquery/heasarc/core.py

+            self._meta_info = self._meta_info[self._meta_info['value'] > 0]
+        return self._meta_info
+
+    def _get_default_columns(self, catalog_name):


Make get_default_columns and get_default_radius consistent, either both private or public depending on whether the end user is expected to use it, but not one of each

bsipocz · 2025-01-11T05:36:58Z

astroquery/heasarc/core.py


-        if url is None:
-            url = conf.server
+    def set_session(self, session):


make it private then? If it's not something the end user should do routinely then better to be tucked away a bit.

bsipocz · 2025-01-11T05:42:35Z

astroquery/heasarc/core.py

+
+        Return
+        ------
+        a list of column names


language nitpicking: The returned output is not a list but a column. Please update the docstring (really, just dropping 'list' is enough, as it can be misleading for someone to expect a python list. Do it here and in the description above, too)

bsipocz · 2025-01-11T05:44:38Z

astroquery/heasarc/core.py

+            (self._meta['table'] == catalog_name)
+            & (self._meta['par'] == '')
+        ]
+        radius = np.double(meta['value'][0]) * u.arcmin


Why using np.double here and not e.g. np.float instead?

bsipocz · 2025-01-11T05:49:02Z

astroquery/heasarc/core.py

+        """
+        return self.list_catalogs(master=False)
+
+    def list_columns(self, catalog_name, full=False):


Why have both get_default_columns and this list_columns methods, practically the default behaviour here could be what get_default_columns is atm, and another keyword could add the more detailed version with the description and unit columns.

bsipocz · 2025-01-11T06:07:27Z

docs/heasarc/heasarc.rst

-The capabilities are currently very limited ... feature requests and contributions welcome!
+There main interface for the Heasarc services``heasarc.Heasac`` now uses
+Virtual Observatory protocols with the Xamin interface, which offers
+more powerful search options than the old Browse interface.


this still stands, no need to mention the old ways, but at least fix the language.

bsipocz · 2025-01-11T06:07:49Z

docs/heasarc/heasarc.rst

+    NGC_3783 60902005 174.7571 -37.7385
+
+To query a region around some position, specifying the search radius,
+we use `~~astropy.units.Quantity`:


Suggested change

we use `~~astropy.units.Quantity`:

we use `~astropy.units.Quantity`:

bsipocz · 2025-01-11T06:08:04Z

docs/heasarc/heasarc.rst

+
+If no radius value is given, a default that is appropriate
+for each catalog is used. You can see the value of the default
+radius values by calling `~~astroquery.heasarc.HeasarcClass.get_default_radius`,


Suggested change

radius values by calling `~~astroquery.heasarc.HeasarcClass.get_default_radius`,

radius values by calling `~astroquery.heasarc.HeasarcClass.get_default_radius`,

bsipocz · 2025-01-11T06:08:13Z

docs/heasarc/heasarc.rst

+passing the name of the catalog.
+
+The list of returned columns can also be given as a comma-separated string to
+`~~astroquery.heasarc.HeasarcClass.query_region`:


Suggested change

`~~astroquery.heasarc.HeasarcClass.query_region`:

`~astroquery.heasarc.HeasarcClass.query_region`:

bsipocz · 2025-01-11T06:08:23Z

docs/heasarc/heasarc.rst

+    13008     1RXS J075526.1+391111 55536.6453587963     Liu 1.4842785992883953
+
+If no columns are given, the call will return a set of default columns.
+If you want all the columns returned, use ``columns='*'```


Suggested change

If you want all the columns returned, use ``columns='*'```

If you want all the columns returned, use ``columns='*'``

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 2 times, most recently 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

zoghbi-a force-pushed the heasarc-refactor branch from 00af79e to b854991 Compare October 17, 2024 18:59

bsipocz requested changes Oct 18, 2024

View reviewed changes

zoghbi-a force-pushed the heasarc-refactor branch 2 times, most recently from 5ecfa95 to 717ba57 Compare January 6, 2025 22:40

zoghbi-a added 2 commits January 9, 2025 13:57

Refactor the heasarc module along with docs and tests

a3c61f3

addressing reviewer comments

1ce3523

zoghbi-a force-pushed the heasarc-refactor branch from 560a2ad to 1ce3523 Compare January 9, 2025 18:57

zoghbi-a force-pushed the heasarc-refactor branch from 42ae40c to dba6380 Compare January 10, 2025 21:26

rename get_datalinks to locate_data following internal feedback

dba6380

bsipocz requested changes Jan 11, 2025

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

zoghbi-a commented Oct 17, 2024

codecov bot commented Oct 17, 2024 •

edited

Loading

bsipocz left a comment

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

zoghbi-a Jan 2, 2025

zoghbi-a commented Jan 9, 2025

bsipocz left a comment

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

bsipocz Jan 11, 2025

	we use `~~astropy.units.Quantity`:
	we use `~astropy.units.Quantity`:

	radius values by calling `~~astroquery.heasarc.HeasarcClass.get_default_radius`,
	radius values by calling `~astroquery.heasarc.HeasarcClass.get_default_radius`,

	`~~astroquery.heasarc.HeasarcClass.query_region`:
	`~astroquery.heasarc.HeasarcClass.query_region`:

	If you want all the columns returned, use ``columns='*'```
	If you want all the columns returned, use ``columns='*'``

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 2025-01-10 21:26:57 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

zoghbi-a commented Oct 17, 2024

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

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

zoghbi-a commented Jan 9, 2025

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

codecov bot commented Oct 17, 2024 •

edited

Loading