-
-
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
Simbad: refactor to use TAP #2954
base: main
Are you sure you want to change the base?
Conversation
Hello @ManonMarchand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-06-05 13:32:14 UTC |
01a826c
to
6b01523
Compare
This comment was marked as outdated.
This comment was marked as outdated.
52edbf9
to
535ddfc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2954 +/- ##
==========================================
+ Coverage 67.35% 67.52% +0.16%
==========================================
Files 236 233 -3
Lines 18320 18224 -96
==========================================
- Hits 12339 12305 -34
+ Misses 5981 5919 -62 ☔ View full report in Codecov by Sentry. |
1fde941
to
e5563ad
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This won't make the cut for 0.4.7, and after that one is out I'm planning to bump the minimum required versions (at least to astropy 5.0, but maybe even something newer), so no need to do workaround for support for old versions. |
We enforce W504 and ignore W503 as break before operator is a bit more legible logic. |
befe1c5
to
585b93d
Compare
585b93d
to
90480f7
Compare
Hello! I'm almost in a un-draft state :) Here are the remaining points that I'm unsure about:
|
d79cddc
to
5ca38bc
Compare
Sorry for the delay in responding:
Next release, 0.4.8. At some point I'll refactor the versioning and related infrastructure, but don't have an ETA when it happens.
I suspect this also run into the same pyvo related RTD failure and was unrelated to your changes. |
Switching back to draft, I'm tweaking a bit the support of the legacy 'query_criteria' |
dd1a61f
to
d3981a5
Compare
- add construct_query method that reads the columns_in_output, join, and criteria attirbutes - support the removed query_criteria method functionnalities by adding a criteria attribute that should be a valid adql clause. The utils CriteriaTranslator can translate between the old and new syntax. - make ROW_LIMIT = -1 to return all lines because TOP 0 or maxrec = 0 are the dedicated way to retrieve table metadata in TAP - fix usage of lru_cache on class methods that can cause memory leaks (see bugbear rule B019)
827da47
to
8ade99d
Compare
ea38351
to
afef848
Compare
Hi all! |
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've done a high-level review of the API changes. I'm putting off the low-level review for now. In brief, there are a few API changes I'm not presently on board with, and at least one that cannot happen in this PR because it's a global astroquery API change.
this commit also adds a patch to simbad's query_objects in the tests
simbad calls lru_cache from python core library, so no cache_location
Co-authored-by: Adam Ginsburg <keflavich@gmail.com>
5a59f20
to
98ae3b4
Compare
I'm still out at a workshop and working towards a deadline this week so it will take a bit of time to get back to this. |
b28bf20
to
c1dfe1a
Compare
@keflavich : I think all your review comments are addressed? |
Hi astroquery,
List of changes:
Adding criteria
Formerly, there was only one way to enter criteria on the output, and it was the
query_criteria
method.This method is now deprecated, and works on a 'reproduce at the best of our capabilities but cannot garantie that everything will work' basis.
But it is replaced by a new
criteria
argument in everyquery_***
method (exceptquery_tap
because people can directly write that into their ADQL string).This new interface looks like this:
Where the criteria string can be translated from the old syntax to the new one with the helper class:
On the output
Adding columns to the output
Some 'votable_fields' options have changed. The old ones should still work and raise a warning indicating the new name in the TAP interface.
That'd look like this:
Every other possibility can be listed with:
(note that the number of possible options went from 107 to 97, among with 12 tables that really don't exist in Simbad since years. So the possibilities are now slightly bigger 🙂 )
Changes in output style
All these could be hidden to the users by modifying the output table in query_tap on python side. Is it worth it?
Empty result
Empty result is valid in TAP. So the default
ROW_LIMIT
could not stay at zero to mean infinity. I copied VizieR module API and went with -1.It also means that a query with no answer returns an empty table and not None like before. This broke JWST module testing, so this PR also has some changes here. Should not break anything, but I'm not an expert of their module.
Caching
Caching in the simbad module is now handled by python built-in
lru_cache
. This might be reverted if we add a caching mechanism to BaseVOQuery (something that'd keep things in a votable-xml format in the default astroquery cache folder maybe?). But I did not go all this way yet.Changes to query_*** methods (except the criteria argument covered above)
Query_object
The
script number ID
inquery_object
is replaced bymatched_id
that contains the ID that corresponded to the wildcard expression.It looks like this:
Query_objects
It now has a
user_specified_id
column as requested in #967 . Theobject_number_id
replacesscript_number_id
(but this could be reverted, it's just strange as there is really one script so I took the liberty to change it)Looks like this:
Note that the requested feature (I could not find the issue anymore, so maybe it's a complaint we received directly in CDS) that objects not found would return an empty line is there: M503 obviously does not exist.
Query_bibcode
The output is very different.
Former output:
New output:
I know it's a very different output but I really hated the former one. I can try to reproduce the old one but☹️
Also, it is now possible to retrieve the abstract with
abstract=True
.Query_region, query_catalog, query_bibobj, query_object_ids
No big changes
In JWST
As Simbad is used in the tests, I just patched what I could how I could, may not be the prettiest way to go :/
Also, now an empty response returns an empty table and not None, so I reflected that in jwst
core.py
.Issues linked to this PR
Fixes: #2198
Fixes: #1468
Partially: #967 (It is done for query_objects, but not for query_region yet)