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 24:1: E302 expected 2 blank lines, found 1
Line 97:13: W503 line break before binary operator
Line 118:13: W503 line break before binary operator
Line 485:17: W503 line break before binary operator
Line 505:13: W503 line break before binary operator

Comment last updated at 2024-10-21 02:03:21 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 50.39062% with 127 lines in your changes missing coverage. Please review.

Project coverage is 67.20%. Comparing base (58f8692) to head (afa7ba2).
Report is 24 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2997      +/-   ##
==========================================
- Coverage   67.49%   67.20%   -0.29%     
==========================================
  Files         233      233              
  Lines       18413    18528     +115     
==========================================
+ Hits        12428    12452      +24     
- Misses       5985     6076      +91

☔ 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.

bsipocz · 2024-10-18T21:03:15Z

astroquery/heasarc/core.py


+@async_to_sync


As this is practically a new module, no need to do this. (I'll clean these up in the upcoming astroquery work, but it's even better if it's not added).

We have started to talk about to ditch the duplication of methods and implement actual async methods (#2598)

bsipocz · 2024-10-18T21:11:11Z

astroquery/heasarc/core.py

 from ..exceptions import InvalidQueryError, NoResultsWarning
 from . import conf

-__all__ = ['Heasarc', 'HeasarcClass']


Please don't remove this, we don't want to expose all of the above imports, etc in the public namespace

Thanks for spotting this. I think it was removed by mistake.

bsipocz · 2024-10-18T21:11:40Z

astroquery/heasarc/core.py

+    Example Usage:
+    ...


Either add an example here or preferably remove these

bsipocz · 2024-10-18T21:11:49Z

astroquery/heasarc/core.py


+@async_to_sync
+class HeasarcClass(BaseVOQuery, BaseQuery):
+    """Class for accessing HEASARC data using XAMIN.


what is XAMIN?

The VO backend. Changed to clarify

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?

bsipocz · 2024-10-18T21:45:56Z

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`:


don't refer to the module, but the classes.

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

docs/heasarc/heasarc.rst

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


This is a private function, don't refer to it in the narrative documentation.

I think this method can be useful to the user, and that is why it is documented. It was initially public, and changed it to private after the first set of comments. I leaning towards making it public again and then referenced it here.

bsipocz · 2024-10-18T21:47:01Z

docs/heasarc/heasarc.rst

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


Why do we skip this?

bsipocz · 2024-10-18T21:47:39Z

docs/heasarc/heasarc.rst

-    >>> from astroquery.heasarc import Heasarc
-    >>> heasarc = Heasarc()
-    >>> table = heasarc.query_object(object_name, mission='rosmaster', radius='120 arcmin')
+    >>> from astroquery.heasarc import Heasac


typo

Suggested change

>>> from astroquery.heasarc import Heasac

>>> from astroquery.heasarc import Heasarc

fixed here and in other places in the rst document.

bsipocz · 2024-10-18T21:48:30Z

docs/heasarc/heasarc.rst

-or a string that can be parsed into one (e.g., '1 degree' or 1*u.degree). The
-following are equivalent:
+Multiple keywords that are separated by space are joined with **AND**, so the 
+following find all tables that have both 'xmm' and 'chandra' keyworkds:


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 and others added 11 commits October 17, 2024 14:55

added HeasarcXaminClass and some tests

a319562

started docs and changed core.py accordingly

0dc783b

expand the download_data functionality

c1d8f82

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

b0aa683

removed build-docs folder which was added by mistake

746f8ea

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

fffc963

update documentation for xamin

68611ba

changes to fix the CI

7648d74

deprecate HeasarcBrowse

6e8b02e

address the first round of review comments

03cf406

fix rebase

b854991

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

zoghbi-a added 2 commits October 17, 2024 15:24

fix flake8 and doc failures

907e50e

add more tests

2270f70

zoghbi-a added 4 commits October 18, 2024 11:58

fix previous commit

8c7160f

another fix

9a5ce5a

fix tables to pass test

91bd43a

fix tests

afa7ba2

bsipocz requested changes Oct 18, 2024

View reviewed changes

zoghbi-a added 2 commits October 20, 2024 21:33

addressing review comments 1

3b53d04

address review comments in heasarc.rst

432fa3e

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

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

bsipocz Oct 18, 2024

zoghbi-a Oct 21, 2024

	>>> from astroquery.heasarc import Heasac
	>>> from astroquery.heasarc import Heasarc


		@async_to_sync

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-10-21 02:03:21 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

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

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