eHST queries and data using TAP and minor fixes in ESA modules by jespinosaar · Pull Request #2597 · astropy/astroquery

jespinosaar · 2022-11-23T15:36:28Z

Hi all,

This pull requests is covering the following issues:

After migrating eHST Science Archive to a new interface, our backend is mainly based on TAP. We have modified the data download to use the data distribution embedded in our TAP.
eHST documentation has been updated.
Added a new method in TapUtils to specify a client ID, useful for statistics.
Issue Rename query_xyz_tap and query() methods to query_tap #2363: method query_hst_tap has been updated to query_tap and deprecation warning is in place.
Issue ESA JWST remote test failues #2590: remote tests failing in eJWST have been fixed.
Remote tests failing in ISO documentation have also been fixed.

Thanks a lot!

cc @esdc-esac-esa-int

codecov · 2022-11-23T15:41:21Z

Codecov Report

Merging #2597 (d5221e5) into main (ef8be74) will increase coverage by 0.00%.
The diff coverage is 79.79%.

@@           Coverage Diff           @@
##             main    #2597   +/-   ##
=======================================
  Coverage   64.20%   64.21%           
=======================================
  Files         130      130           
  Lines       16850    16885   +35     
=======================================
+ Hits        10819    10842   +23     
- Misses       6031     6043   +12

Impacted Files	Coverage Δ
astroquery/esa/hubble/core.py	`83.63% <78.26%> (-2.84%)`	⬇️
astroquery/esa/jwst/core.py	`75.43% <100.00%> (ø)`
astroquery/utils/tap/core.py	`44.55% <100.00%> (+0.21%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jespinosaar · 2022-11-23T16:30:40Z

Everything seems to be working, please let me know if you have further comments. Thanks!

bsipocz

Over this looks to be almost ready to go, all remote tests pass, etc.

I've left some minor comments, and some that may point to a follow-up PR.

bsipocz · 2022-11-23T18:11:48Z

+# Licensed under a 3-clause BSD style license - see LICENSE.rst
 """
-
-@author: Javier Duran


This reminds me that we need to sort out a team page listing ESA as a significant contributor. It's not forgotten, we just haven't got to it yet.

Many thanks! just ping me if you need a list of users or something else.

bsipocz · 2022-11-23T18:27:59Z

+        assert "This target name cannot be determined with this resolver: ALL" in err.value.args[0] or "Failed "
+        "to parse" in err.value.args[0]


You need parens around the linebreak otherwise you only assert on the string "Failed " (which is always True) rather than on "Failed to parse" in err.value.args[0].

Do the same for all the other similar lines below.

Suggested change

assert "This target name cannot be determined with this resolver: ALL" in err.value.args[0] or "Failed "

"to parse" in err.value.args[0]

assert "This target name cannot be determined with this resolver: ALL" in err.value.args[0] or ("Failed "

"to parse" in err.value.args[0])

bsipocz · 2022-11-23T18:30:06Z

            flag to display information about the process
        product_type : string
            type of product retrieval, optional
-            PRODUCT, SCIENCE_PRODUCT or POSTCARD


I wonder whether we need some extra docs, or deprecation that notifies anyone using these product types that things have changed?

Also, could you update the download_product usage in the docs to not use e.g. SCIENCE_PRODUCT?
(I see that the example still works, but I suppose it would be nice to clean up nevertheless)

I have updated the examples with the new parameters and added some deprecation messages in the code and the documentation.

bsipocz · 2022-11-23T18:37:48Z

+        return filename

-    def get_postcard(self, observation_id, calibration_level="RAW",
+    def get_postcard(self, observation_id, *, calibration_level="RAW",


Thanks for doing this.

changing kwargs to be keyword only needs to be mentioned in a new changelog entry, look for copy-paste examples.

Also, I wonder, while you're at it, could you do this for all the methods?

bsipocz · 2022-11-23T22:08:55Z

-                                       dump_to_file=output_file is not None)
-        table = job.get_results()
-        return table
+        warnings.warn(


Doing it with the deprecated decorator from astropy.utils may be a tiny bit simpler, but at this point you don't need to change this.

I have also included the decorator, but I left the warning.

@deprecated ensures that a deprecation warning is emitted, so the warnings raised by the decorated function itself are duplicates.

Done, commented below.

bsipocz · 2022-11-23T22:29:01Z

        else:
            return tables

+    def get_status_messages(self):


you may want to mention this in the changelog or in the docs of all modules having it (get_status_messages is mentioned in the docs for the JWST module, but not for the rest)

It is mentioned now.

bsipocz · 2022-11-23T22:30:16Z

+        table = job.get_results()
+        return table
+
    def query_hst_tap(self, query, async_job=False, output_file=None,


This deprecation needs to be mentioned in the changelog, preferably as a separate entry.

Deprecation included in the changelog. I have also ordered the different entries in the appropriate categories.

bsipocz · 2022-11-23T22:30:46Z

  a lot faster. [#2524]

- Update TAP url to avoid 301 HTTPError. [#2567]
+- Update to TAP url to query data and download files, aligned with the new eHST Science Archive. [#2567][#2597]


more changelog entries are needed for this PR to cover the various changes, see the comments below.

Changelog entries added. Please let me know if you think more are required.

bsipocz · 2022-11-23T22:33:21Z

        assert "Coordinates must be either a string or " \
               "astropy.coordinates" in err.value.args[0]

-    def test_query_hst_tap(self):


could you keep one query_hst_tap test, too to test whether the deprecation warning is issued?

Added again and being executed.

bsipocz · 2022-11-23T22:41:14Z


    def test_download_product_errors(self):
-        ehst = ESAHubbleClass(self.get_dummy_tap_handler())
+        ehst = ESAHubbleClass(tap_handler=self.get_dummy_tap_handler(), show_messages=False)


Can we have one show_messages=True test, too (with mocking some status messages)?

Test included.

jespinosaar · 2022-11-24T08:00:32Z

Thanks for your comments @bsipocz ! working on them.

jespinosaar · 2022-11-24T11:06:55Z

I think I have answered all your comments @bsipocz , please let me know if further changes are required.

jespinosaar · 2022-11-24T11:28:00Z

Don't know why the docs is taking so long...

eerovaher · 2022-11-24T14:57:46Z

+    @deprecated(since="0.4.7", message=("Use of query_hst_tap method is no longer supported. "
+                "Please use query_tap method instead, with the same arguments."),
+                alternative="query_tap")


message does not have to be specified. The following works:

Suggested change

@deprecated(since="0.4.7", message=("Use of query_hst_tap method is no longer supported. "

"Please use query_tap method instead, with the same arguments."),

alternative="query_tap")

@deprecated(since="0.4.7", alternative="query_tap")

The resulting warning is

WARNING: AstropyDeprecationWarning: The query_hst_tap function is deprecated and may be removed in a future version. Use query_tap instead.

Ok, I have removed the AstropyDeprecationWarning

eerovaher · 2022-11-24T15:14:29Z

+    @staticmethod
+    def __set_client_id(client_id):
+        if client_id:
+            global TAP_CLIENT_ID


According to Python naming conventions TAP_CLIENT_ID is supposed to be (and so far has been) a module level constant. Now it should be changed to be an instance attribute instead.

Modified TAP_CLIENT_ID to be an instance attribute.

jespinosaar · 2022-11-24T16:40:08Z

Ok, new comments have been implemented, thanks @bsipocz and @eerovaher !

bsipocz · 2022-11-26T07:56:36Z

@jespinosaar - Thanks for the PR and the very quick review cycle!

I did a minor rebase as there were two merge/duplicated commits. Github quicklook however now wrongly shows these as my commits, you may want to add your ext.esa.int email address to your github profile so they will be properly picked up by the web interface for you (git log showing them correctly, it's just the web interface issue).

This was referenced Nov 23, 2022

ESA JWST remote test failues #2590

Closed

Rename query_xyz_tap and query() methods to query_tap #2363

Open

jespinosaar added this to the v0.4.7 milestone Nov 23, 2022

jespinosaar added 6 commits November 23, 2022 16:54

EHSTPCR-1071: correct tapplus and data distribution

dcc881f

Fix for astropy#2590

ac39c9c

Custom client_id in TapPlus

462ea9b

Fixes in remote tests

023eaec

EHSTPCR-1071: style fixes

22a15e7

changes.rst file updated

7aa111c

jespinosaar force-pushed the ESA_eHST-tap_https_url branch from a4581ff to 7aa111c Compare November 23, 2022 15:56

EHSTPCR-1071: fix in test cov

7180126

jespinosaar self-assigned this Nov 23, 2022

jespinosaar added esa.esa_hubble esa.jwst esa.iso labels Nov 23, 2022

bsipocz mentioned this pull request Nov 23, 2022

API discussion: keeping duplicated methods xyz/xyz_async vs having an async kwarg? #2598

Open

bsipocz reviewed Nov 23, 2022

View reviewed changes

bsipocz linked an issue Nov 23, 2022 that may be closed by this pull request

ESA JWST remote test failues #2590

Closed

jespinosaar added 3 commits November 24, 2022 11:47

eHST: tests and doc updated, kwargs methods

0001244

eJWST: format improvements

f417fc0

eHST: pycodestyle issues with unused dependencies

d126fd9

eerovaher reviewed Nov 24, 2022

View reviewed changes

bsipocz approved these changes Nov 26, 2022

View reviewed changes

jespinosaar added 4 commits November 25, 2022 23:51

eHST: remove warning in deprecated method, tap_client as variable

11fcb7c

TAP_CLIENT_ID replaced with tap.tap_client_id

3569a03

TAP_CLIENT_ID removed

141ce72

HSC version removed in docs

d5221e5

bsipocz force-pushed the ESA_eHST-tap_https_url branch from c839a77 to d5221e5 Compare November 26, 2022 07:52

bsipocz merged commit f59362b into astropy:main Nov 26, 2022

bsipocz mentioned this pull request Nov 27, 2022

Use keyword only arguments #1746

Closed

62 tasks

		assert "This target name cannot be determined with this resolver: ALL" in err.value.args[0] or "Failed "
		"to parse" in err.value.args[0]

Uh oh!

Uh oh!

Conversation

jespinosaar commented Nov 23, 2022

Uh oh!

codecov Bot commented Nov 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jespinosaar commented Nov 23, 2022

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jespinosaar commented Nov 24, 2022

Uh oh!

jespinosaar commented Nov 24, 2022

Uh oh!

jespinosaar commented Nov 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jespinosaar commented Nov 24, 2022

Uh oh!

bsipocz commented Nov 26, 2022

Uh oh!

codecov Bot commented Nov 23, 2022 •

edited

Loading