Skip to content
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

Enable TAP for ESO module #3141

Draft
wants to merge 90 commits into
base: main
Choose a base branch
from
Draft

Enable TAP for ESO module #3141

wants to merge 90 commits into from

Conversation

juanmcloaiza
Copy link
Contributor

@juanmcloaiza juanmcloaiza commented Nov 28, 2024

Addresses #3138

Changes:

  • list_instruments
  • list_surveys list_collections
  • query_instruments
  • query_surveys query_collections
  • query_main
  • query_apex_quicklooks
  • deprecate functions *_surveys(...) in favor of _*collections(...)
  • Make sure that ALMA collection is handled properly
  • Add type hints to all methods / functions

@pep8speaks
Copy link

pep8speaks commented Nov 28, 2024

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-17 14:13:03 UTC

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 50.54152% with 137 lines in your changes missing coverage. Please review.

Project coverage is 68.49%. Comparing base (3e9ff61) to head (965d540).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/eso/core.py 43.75% 126 Missing ⚠️
astroquery/eso/utils.py 78.84% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3141      +/-   ##
==========================================
+ Coverage   68.34%   68.49%   +0.14%     
==========================================
  Files         231      232       +1     
  Lines       19190    19135      -55     
==========================================
- Hits        13116    13106      -10     
+ Misses       6074     6029      -45     

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

@bsipocz bsipocz added the eso label Nov 28, 2024
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment for now regarding the deprecation, otherwise is looking good so far (I didn't dive into the details, but had a quick look

astroquery/eso/core.py Outdated Show resolved Hide resolved
astroquery/eso/core.py Outdated Show resolved Hide resolved
astroquery/eso/core.py Outdated Show resolved Hide resolved
astroquery/eso/core.py Outdated Show resolved Hide resolved
astroquery/eso/core.py Outdated Show resolved Hide resolved
try:
table_to_return = tap.search(query_str).to_table()
except pyvo.dal.exceptions.DALQueryError:
raise pyvo.dal.exceptions.DALQueryError(f"\n\nError executing the following query:\n\n{query_str}\n\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why rethrowing the same exception with a different error message? Is the original error message not clear enough? I think it's also possible to chain the original exception with a custom one but I don't remember the syntax

Copy link
Contributor Author

@juanmcloaiza juanmcloaiza Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is helpful to see the query that was being executed. In that way users can test it externally (for example here: http://archive.eso.org/programmatic/#TAP) and pinpoint the source of error. I found this to be the simplest way of achieving that goal, but I'm open to suggestions, if there's a better or standard way to do it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One could wonder if this shouldn't be addressed in TAP itself -> after all TAP could return the query as part of the error message.

astroquery/eso/core.py Outdated Show resolved Hide resolved
astroquery/eso/core.py Outdated Show resolved Hide resolved
@juanmcloaiza juanmcloaiza force-pushed the TAP branch 2 times, most recently from eb83ef2 to 1829e1c Compare January 17, 2025 12:55
@juanmcloaiza
Copy link
Contributor Author

@bsipocz, hi, I'd like to ask for your feedback regarding deprecated functionality.

This PR's scope is to switch the back-end of the ESO module from WDB to TAP. In doing so, many of the functions will be renamed and their signatures changed (i.e. completely new functions are being written, while others are being removed).

If I understand correctly, astroquery requires that deprecated features are handled via @deprecated decorators. However, in this particular case, the changes are rather large --to some extent a @deprecated decorator would need to be added to the whole ESO module (take this last phrase with a grain of salt, I'm exaggerating).

The question I'd like to ask you as an astroquery maintainer is the following: Would it be admissible for this time to avoid adding deprecated decorators to each of the functions and arguments that we are discontinuing? My impression is that the @deprecated benefits will be small in comparison to the illegibility of the code they'll produce. I would prefer instead to make it very explicit in the documentation that whatever functionality available in past versions may not be available from this version on.

Please let us know what you think (cc @szampier). Thanks in advance for your feedback ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants