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: query_sia and query_ssa to return QTable #3252

Merged
merged 4 commits into from
Mar 17, 2025

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Mar 13, 2025

This addresses the easy part of #3244

I'm doing TAP separately to see if we need some extra tip-toeing.

@bsipocz bsipocz added this to the v0.4.10 milestone Mar 13, 2025
@bsipocz bsipocz requested a review from andamian March 13, 2025 16:43
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 69.08%. Comparing base (7494746) to head (fe2614f).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
astroquery/ipac/irsa/core.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3252      +/-   ##
==========================================
- Coverage   69.09%   69.08%   -0.01%     
==========================================
  Files         232      232              
  Lines       19637    19639       +2     
==========================================
  Hits        13568    13568              
- Misses       6069     6071       +2     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andamian
Copy link

This looks good and it's the right thing to do.
My only concern is that we are breaking the backwards compatibility without warning (and we are not using type hints either to allow at least IDEs to issue warnings). What I did for the other queries in alma (and honestly I don't know how I've missed query_sia) was to add a flag enhanced_results to control the transition of the returned type. Right now it's optional but we can change it to issue a warning and eventually it can change the default to true and error when false. Note that getting rid of this optional attribute at the end also constitutes an API change but at least that it's more obvious.
Speaking of alma.query_sia - I would suggest using the same mechanism as the other alma query methods. In alma we actually go a bit further with the enhanced table and besides table we also try to make something useful of the s_region field. That unfortunately introduces an optional dependency (astropy.regions). If that feature is useful to irsa as well, this feature this could be a case for turning it into a mandatory dependency. I don't know how good of an idea this is @keflavich?

@bsipocz
Copy link
Member Author

bsipocz commented Mar 17, 2025

OK, that's all reasonable. My main concern is to return pyvo tables rather than if it's a Table or a QTable.

We also just very recently added query_sia and query_ssa to IRSA, so I can go ahead with PR as is and just mention the API change in the changelog. I think it will have way less user impact than waiting to do it in the next one.
As for TAP, again, my main concern is to have pyvo classes returned and then the user code looks like pyvotable.to_table().to_pandas() at multiple places in the code. They almost always don't need access to the pyvotable (I say almost always, as I'm sure they can be a corner case that I haven't yet come across, but in practice, I haven't yet seen a case where they needed it).

So, my question: can we merge this as is, or you rather have me just do IRSA for now and leave the alma part for the next release?

@andamian
Copy link

My impression is that alma is already used extensively and I'd rather take a gradual approach to upgrading it. Not great to have similar but different APIs but I don't want to inconvenience too many users. So please do just irsa for now. I will have to open a ticket and add the enhanced_table attribute to alma.query_sia.

@bsipocz bsipocz force-pushed the ENH_table_instead_votable_sia branch from cf5f423 to fe2614f Compare March 17, 2025 19:00
@bsipocz bsipocz merged commit 78aee6f into astropy:main Mar 17, 2025
10 of 12 checks passed
@bsipocz bsipocz deleted the ENH_table_instead_votable_sia branch March 17, 2025 19:14
@bsipocz
Copy link
Member Author

bsipocz commented Mar 17, 2025

OK, the alma part has been pulled out into #3261.

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.

3 participants