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

ENH: ipac.irsa backend refactoring to use TAP rather than Gator #2823

Merged
merged 12 commits into from Sep 13, 2023

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Sep 6, 2023

This refactoring was long planned, to switch out the Gator backend to TAP.

Most of the things work already, but some wont:

TODO, and other comments:

  • either move the old GATOR approach to a separate file and import the old methods as deprecated, or write a proper async, preferably with a keyword. I would not consider either of these a blocker. ==> decided to remove them as is for now. We may add proper async later.
  • The TAP server doesn't seem to support Box searches, so I skipped the tests for now. If box need to stay, maybe add support for it via a polygon search. Or wait for upstream to support it (after all astroquery users can write their own polygon, or use Cone instead. ==> This has been reported upstream, so there is nothing to do here. I temporarily removed the box example from the narrative docs.
  • Test coverage is not great, the local tests should be meaningful, e.g. testing the generated ADQL query?
  • More coverage will be needed to cover the other keywords
    • selcol
    • get_query_payload
    • respect row_limit
    • respect timeout
  • cache need to be deprecated as a kwarg as pyvo doesn't do cache. I suppose it could consider it as a future feature, but it's way beyond the scope here. ==> cache and verbose are now deprecated
  • SIA is to be added in a follow-up PR as it doesn't have any legacy to swap out, and then it would also not grow this PR to an unmanagable size.
  • not a blocker for this, but in a follow-up: vectorize queries with table upload
  • update narrative docs

TODO for coordinating API:

  • selcols kwarg need to be renamed. E.g. elsewhere we use columns, but it has to be double check what the most frequent name is
  • the list_catalogs should return the full info of available catalogs, not just the name and description. Or have a way to return all info, maybe with a kwarg. it needs to be double checked what the most often used method name for the same or similar functionality ==> added 'full' as a kwarg to get the full VOTable.

This should close #2382 (needs to be checked)
And I suppose there is a TAP-based solution for #1018, too

Closes fornax-navo/fornax-demo-notebooks#73

@bsipocz bsipocz added this to the v0.4.7 milestone Sep 6, 2023
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #2823 (1750798) into main (51316d7) will decrease coverage by 0.06%.
Report is 4 commits behind head on main.
The diff coverage is 72.88%.

@@            Coverage Diff             @@
##             main    #2823      +/-   ##
==========================================
- Coverage   66.40%   66.34%   -0.06%     
==========================================
  Files         235      235              
  Lines       18140    18077      -63     
==========================================
- Hits        12046    11994      -52     
+ Misses       6094     6083      -11     
Files Changed Coverage Δ
astroquery/ipac/irsa/core.py 75.00% <72.41%> (-2.62%) ⬇️
astroquery/ipac/irsa/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@bsipocz bsipocz marked this pull request as ready for review September 7, 2023 04:04
@bsipocz
Copy link
Member Author

bsipocz commented Sep 7, 2023

I've removed the draft status as some of the remaining TODOs could use some input. E.g. how do you feel about renaming selcols to columns? Also, what about dropping Box, or at least falling back to Cone instead?

@keflavich
Copy link
Contributor

renaming selcols to columns

👍

Also, what about dropping Box, or at least falling back to Cone instead?

Box searches are very useful, so I'd prefer to keep them available if possible.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 7, 2023

Ok, then I'll open a ticket for it upstream and keep it in the code here (I think the current error message is clear enough, though as a temporary measure, happy to fall back on Cone.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 13, 2023

@keflavich - This is ready for review. The remaining TODO items will be follow-ups (SIA and vectorized queries), and I would also address the config item ones separately as we have a few outstanding PRs in that regard, so we may want to do the same approach here, too.
Also, we have an internal ticket about the Box search so I opted for not adding a workaround here, but temporarily removed the narrative docs about it. We can revise these statuses at release time.

@alaity47 - let me know if someone wants to have a look at this internally.

Copy link
Contributor

@keflavich keflavich left a comment

Choose a reason for hiding this comment

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

LGTM. I mostly looked at the additions, not the removals, and I didn't see any clear problems. I don't speak ADQL very fluently, though.

astroquery/ipac/irsa/core.py Show resolved Hide resolved
@bsipocz bsipocz merged commit 3b2110f into astropy:main Sep 13, 2023
9 of 10 checks passed
@bsipocz bsipocz deleted the irsa_tap branch December 8, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IRSA Astroquery doesn't return full COSMOS field catalog Large Gator queries timeout or return emtpy tables
2 participants