Simbad: refactor to use TAP #2954
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-04 08:08:45 UTC |
ManonMarchand
force-pushed
the
refactor_Simbad
branch
2 times, most recently
from
01a826c
to
6b01523
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ManonMarchand
force-pushed
the
refactor_Simbad
branch
2 times, most recently
from
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. |
ManonMarchand
force-pushed
the
refactor_Simbad
branch
from
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. |
ManonMarchand
force-pushed
the
refactor_Simbad
branch
5 times, most recently
from
befe1c5
to
585b93d
Compare
ManonMarchand
force-pushed
the
refactor_Simbad
branch
from
585b93d
to
90480f7
Compare
|
Hello! I'm almost in a un-draft state :) Here are the remaining points that I'm unsure about:
|
ManonMarchand
force-pushed
the
refactor_Simbad
branch
3 times, most recently
from
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' |
ManonMarchand
force-pushed
the
refactor_Simbad
branch
from
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)
these are the files of the simbad.utils.CriteriaTranslator parser
and move from os to pathlib
ManonMarchand
force-pushed
the
refactor_Simbad
branch
3 times, most recently
from
827da47
to
8ade99d
Compare
ManonMarchand
force-pushed
the
refactor_Simbad
branch
from
ea38351
to
afef848
Compare
|
Hi all! |
| - all query methods now accept ``get_adql`` boolean argument that returns the ADQL string | ||
| instead of sending the request to SIMBAD. The ``verbose`` and ``get_query_payload`` | ||
| are removed from all methods [#2954] |
There was a problem hiding this comment.
@bsipocz big picture question here - might it be better to keep get_query_payload as a duplicate of get_adql here? I liked that this value was consistent across all modules. It's primarily for debugging, so I think we're the primary customers, but I'd like your take on this.
There was a problem hiding this comment.
I'm ok with keeping both too, it's just that get_query_payload will return the ADQL string instead of the payload. Maybe a true request payload is something to add in PyVO's TAP module ?
There was a problem hiding this comment.
Right, if it's the payload, it's going to be something like {'adql': <text of adql query>}.
It's OK if it's just the ADQL, though. The payloads tend to be different for every module, though I think they are all dicts.
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>
ManonMarchand
force-pushed
the
refactor_Simbad
branch
from
5a59f20
to
98ae3b4
Compare
Hi astroquery,
List of changes:
Adding criteria
Formerly, there was only one way to enter criteria on the output, and it was the
query_criteriamethod.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
criteriaargument in everyquery_***method (exceptquery_tapbecause 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_LIMITcould 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 IDinquery_objectis replaced bymatched_idthat contains the ID that corresponded to the wildcard expression.It looks like this:
Query_objects
It now has a
user_specified_idcolumn as requested in #967 . Theobject_number_idreplacesscript_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)