-
-
Notifications
You must be signed in to change notification settings - Fork 390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
eHST queries and data using TAP and minor fixes in ESA modules #2597
eHST queries and data using TAP and minor fixes in ESA modules #2597
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2597 +/- ##
=======================================
Coverage 64.20% 64.21%
=======================================
Files 130 130
Lines 16850 16885 +35
=======================================
+ Hits 10819 10842 +23
- Misses 6031 6043 +12
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
a4581ff
to
7aa111c
Compare
Everything seems to be working, please let me know if you have further comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
""" | ||
|
||
@author: Javier Duran |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks! just ping me if you need a list of users or something else.
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -78,39 +70,43 @@ def download_product(self, observation_id, *, calibration_level=None, | |||
flag to display information about the process | |||
product_type : string | |||
type of product retrieval, optional | |||
PRODUCT, SCIENCE_PRODUCT or POSTCARD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether we need some extra docs, or deprecation that notifies anyone using these product types that things have changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the examples with the new parameters and added some deprecation messages in the code and the documentation.
|
||
def get_postcard(self, observation_id, calibration_level="RAW", | ||
def get_postcard(self, observation_id, *, calibration_level="RAW", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this.
changing kwargs to be keyword only needs to be mentioned in a new changelog entry, look for copy-paste examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I wonder, while you're at it, could you do this for all the methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
astroquery/esa/hubble/core.py
Outdated
dump_to_file=output_file is not None) | ||
table = job.get_results() | ||
return table | ||
warnings.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have also included the decorator, but I left the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deprecated
ensures that a deprecation warning is emitted, so the warnings raised by the decorated function itself are duplicates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, commented below.
@@ -717,6 +746,22 @@ def get_tables(self, only_names=True, verbose=False): | |||
else: | |||
return tables | |||
|
|||
def get_status_messages(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is mentioned now.
astroquery/esa/hubble/core.py
Outdated
dump_to_file=output_file is not None) | ||
table = job.get_results() | ||
return table | ||
|
||
def query_hst_tap(self, query, async_job=False, output_file=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecation needs to be mentioned in the changelog, preferably as a separate entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation included in the changelog. I have also ordered the different entries in the appropriate categories.
CHANGES.rst
Outdated
@@ -32,7 +32,7 @@ esa.hubble | |||
- Refactored query_criteria to use ehst.archive table therefore making the query | |||
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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more changelog entries are needed for this PR to cover the various changes, see the comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changelog entries added. Please let me know if you think more are required.
@@ -194,7 +195,7 @@ def test_cone_search_coords(self): | |||
assert "Coordinates must be either a string or " \ | |||
"astropy.coordinates" in err.value.args[0] | |||
|
|||
def test_query_hst_tap(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you keep one query_hst_tap
test, too to test whether the deprecation warning is issued?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added again and being executed.
@@ -86,19 +84,19 @@ def get_dummy_tap_handler(self): | |||
return dummyTapHandler | |||
|
|||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have one show_messages=True
test, too (with mocking some status messages)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test included.
Thanks for your comments @bsipocz ! working on them. |
I think I have answered all your comments @bsipocz , please let me know if further changes are required. |
Don't know why the docs is taking so long... |
astroquery/esa/hubble/core.py
Outdated
@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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message
does not have to be specified. The following works:
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have removed the AstropyDeprecationWarning
astroquery/utils/tap/core.py
Outdated
@staticmethod | ||
def __set_client_id(client_id): | ||
if client_id: | ||
global TAP_CLIENT_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified TAP_CLIENT_ID to be an instance attribute.
Ok, new comments have been implemented, thanks @bsipocz and @eerovaher ! |
c839a77
to
d5221e5
Compare
@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 |
Hi all,
This pull requests is covering the following issues:
Thanks a lot!
cc @esdc-esac-esa-int