Feature simbad query tap #2856

ManonMarchand · 2023-10-19T13:43:45Z

Hello,

What this PR does

Add Simbad.query tap functionality to the simbad module. This is called like this:

from astroquery.simbad import Simbad
Simbad.query_tap("SELECT top 5 from ref")

This feature comes with helpers methods to explore SIMBAD's relational schema.

Simbad.list_tables() returns the public tables of SIMBAD
Simbad.list_columns(*args, keyword: str = None) returns the columns corresponding to the entry table(s) and with matches for the given keyword
Simbad.list_linked_tables(table: str) returns the non obvious (i.e. the columns don't have the same names) possible joins

Properties to connect to the tap endpoint:

Simbad.simbad_mirrors is a set of the two simbad's mirrors
Simbad.mirror takes the value of conf.server at instantiation but can be switched later
Simbad.tap is created from the current Simbad.mirror

And the private method

_adql_parameter
that will be called when building adql queries from user inputs (to silently escape invalid characters and reserved sql vocabulary)

Documentation-wise

I took the liberty to add a graphviz graph of simbad's schema since sphinx-graphviz is already in the dependencies of sphinx-automodapi but we can remove it if needed.

The documentation on queries are now organized in a rough

everything about objects
everything about bibliography
queries that mix both
This might need discussion too ?

That was also a lot of text and I'm not a native-english speaker, if someone has the time to point mistakes/unclear parts I'd love feedback.

Next steps

This PR only adds new methods. The rework to solve #1468 will be based on Simbad.query_tap and the two private functions introduced here.

codecov · 2023-10-19T13:49:36Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (885734b) 66.53% compared to head (f776ef8) 66.56%.
Report is 110 commits behind head on main.

❗ Current head f776ef8 differs from pull request most recent head ccf93cb. Consider uploading reports for the commit ccf93cb to get more accurate results

Files	Patch %	Lines
astroquery/simbad/core.py	80.64%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
+ Coverage   66.53%   66.56%   +0.03%     
==========================================
  Files         235      235              
  Lines       18114    18162      +48     
==========================================
+ Hits        12052    12090      +38     
- Misses       6062     6072      +10

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

ManonMarchand · 2023-10-19T13:50:44Z

From the oldest dependencies test, it looks like DALOverflowWarning was introduced in this commit astropy/pyvo@82ed24d in pyvo v1.4.2 should I avoid using it?

pep8speaks · 2023-10-19T14:09:42Z

Hello @ManonMarchand! 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 2024-01-26 16:41:53 UTC

ManonMarchand · 2023-10-19T14:28:43Z

On the coverage, some lines are marked as missed but are tested in test_simbad_remote.py, is it normal?

bsipocz · 2023-10-19T21:07:14Z

Thank you so much! I'll try to get a timely review for this.

To answer the questions above:

On the coverage, some lines are marked as missed but are tested in test_simbad_remote.py, is it normal?

yes, do ignore the number here, its main purpose is to check whether there is a significant drop in coverage and we are aware that the actual percentage is most often larger when one includes the remote tests (I tend to run those numbers for significantly big PRs like this one as part of the review).

Re pyvo 1.4.2: that version is a bit too new to mandate as a minimum required version. Maybe you could do a try/except import and later a version dependent conditional?

ManonMarchand · 2023-10-20T12:52:47Z

I added the try/except clause for DALOverflowWarning 🙂

edit: the new failure does not seem related

bsipocz

I made a couple of comments while travelling, will come back for another fine combing before merging, but honestly I don't think anything else critical will be found.

astroquery/simbad/core.py

astroquery/simbad/tests/test_simbad_remote.py

docs/simbad/query_tap.rst

ManonMarchand · 2023-11-09T14:03:17Z

Oups, I was squashing my commits and rebased too far sorry. I have no idea how to fix that

bsipocz · 2023-11-09T14:17:58Z

Oups, I was squashing my commits and rebased too far sorry. I have no idea how to fix that

I suspect you rebased on an outdated commit, rather than the freshly fetched main branch from the astropy/upstream remote.

ManonMarchand · 2023-11-09T14:19:32Z

Will it be an issue when merging?

bsipocz · 2023-11-09T14:27:48Z

Will it be an issue when merging?

No, I'll do a rebase before merge ;)

ManonMarchand · 2023-11-10T15:30:24Z

I added caching on query_tap in today's commits (sorry for changing while you were reviewing). It only works when there are no uploaded tables in the query parameters since astropy tables are not hashable (and thus cannot be parameters in a function wrapped in lru_cache). I still think it can be very useful: uploaded tables are quite rare.

It also made me realize that there were no examples of uploaded table in the narrative docs so I made one up.

EDIT : and I'll stop changing stuff so that you can review, sorry again.

astroquery/simbad/core.py

bsipocz · 2023-11-10T20:38:03Z

EDIT : and I'll stop changing stuff so that you can review, sorry again.

No worries, do add as many changes as you wish, you can always add the review label, or ping me when done and I'll come back then.

ManonMarchand · 2023-11-13T12:57:12Z

No worries, do add as many changes as you wish, you can always add the review label, or ping me when done and I'll come back then.

All done :)

keflavich

I have a couple nitpicky English comments, but this looks great and the documentation is super helpful.

docs/simbad/query_tap.rst

docs/simbad/simbad.rst

ManonMarchand · 2023-12-18T10:06:08Z

All done for Adam comments

SimbadClass.query_tap wraps the pyvo.dal.TAPService.run_async method. This commit also add the SimbadClass.simbad_mirrors and SimbadClass.tap attributes.

queries are now in three sections: - objects-related queries - bibliography-related queries - mixed type queries

this commit also adds two private methods useful when building queries from users input: _adql_parameter and _adql_name.

we also add a graphviz file that represents a quick view of simbad's tables

list_columns now accept a 'keyword' keyword argument to filter the output columns find_linked_tables is also renamed to list_linked_tables for homogenity

The caching decorator could not be applied for the uploaded tables because the astropy table objects are non-hashable. This implementation only caches calls without uploads as I could not find a workaround.

Co-authored-by: Adam Ginsburg <[email protected]>

a script to regenerate the DOT file is here : https://github.com/ManonMarchand/simbad-ER-diagram/blob/main/simbad-and-graphviz.py

some fields have changed their datatype upstream following astropy#2916

ManonMarchand · 2024-01-26T17:01:59Z

I removed the _adql_name function because it didn't prove useful in the next steps of astroquery.simbad's refactoring.

bsipocz

Thank you @ManonMarchand, this should go in now.

ManonMarchand force-pushed the feature_Simbad_query_TAP branch 2 times, most recently from a9dce73 to 9d0abcb Compare October 19, 2023 14:22

ManonMarchand added the simbad label Oct 19, 2023

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from 9d0abcb to 7769e25 Compare October 19, 2023 14:43

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from 3cfe840 to ba23c2c Compare October 20, 2023 13:08

bsipocz added this to the v0.4.7 milestone Nov 8, 2023

bsipocz reviewed Nov 8, 2023

View reviewed changes

ManonMarchand force-pushed the feature_Simbad_query_TAP branch 3 times, most recently from 16784cf to b86d6aa Compare November 9, 2023 14:01

bsipocz reviewed Nov 10, 2023

View reviewed changes

astroquery/simbad/core.py Outdated Show resolved Hide resolved

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from f658c8d to 01256a3 Compare November 13, 2023 12:48

ManonMarchand added the to review label Nov 13, 2023

keflavich approved these changes Nov 21, 2023

View reviewed changes

This was referenced Jan 5, 2024

TapPlus parse simbad fail with non ascii char #2916

Closed

Unfriendly Simbad.query_criteria functionality #2041

Closed

ManonMarchand and others added 19 commits January 26, 2024 10:49

FEAT: Add a query_tap method to SimbadClass

eb79ed7

SimbadClass.query_tap wraps the pyvo.dal.TAPService.run_async method. This commit also add the SimbadClass.simbad_mirrors and SimbadClass.tap attributes.

docs: reorganize sections in simbad docs

e6580a8

queries are now in three sections: - objects-related queries - bibliography-related queries - mixed type queries

docs: add query TAP section

ab7c7d2

feat: add helper functions to explore SIMBAD tables

f003c5d

this commit also adds two private methods useful when building queries from users input: _adql_parameter and _adql_name.

docs: add documentation for query_tap and its helper methods

fca6d15

we also add a graphviz file that represents a quick view of simbad's tables

test: add tests for _adql_parameter and _adql_name

14fe47d

docs: add changelog entry

9076760

test: add try/except on pyvo version for DALOverflowWarning

6d31b1f

fix: exchange order of BaseVOQuery and SimbadBaseQuery

564517a

refactor: rename tables and columns into list_tables and list_columns

96777c1

list_columns now accept a 'keyword' keyword argument to filter the output columns find_linked_tables is also renamed to list_linked_tables for homogenity

style: make optional kwargs kwarg only

67aab66

docs: remove links to issues, rephrase adql example

cc3aeab

docs: remove doctest ignore on first TAP example

21ccd2b

feat: add caching to query_tap and hardlimit

744262f

The caching decorator could not be applied for the uploaded tables because the astropy table objects are non-hashable. This implementation only caches calls without uploads as I could not find a workaround.

docs: add an example of Simbad.query_tap with an uploaded table

821aec7

fix: remove use of functools.cache_property for python 3.7 support

8ff6d73

fix: switch simbad.mirror list into simbad.conf.server_list

4983ab6

docs: fix english issues

79d4010

Co-authored-by: Adam Ginsburg <[email protected]>

docs: increase font size in simbad graph

56c8e1d

a script to regenerate the DOT file is here : https://github.com/ManonMarchand/simbad-ER-diagram/blob/main/simbad-and-graphviz.py

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from f776ef8 to f446183 Compare January 26, 2024 13:16

ManonMarchand added 3 commits January 26, 2024 14:19

fix: updated datatype in doctest outputs

b99ad4c

some fields have changed their datatype upstream following astropy#2916

edit intersphinx links

d2b2bc4

fix: simplify IN statement

1304824

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from f446183 to 1304824 Compare January 26, 2024 13:20

fix: remove unused adql_name function

ccf93cb

ManonMarchand force-pushed the feature_Simbad_query_TAP branch from b6cc855 to ccf93cb Compare January 26, 2024 16:41

bsipocz approved these changes Jan 26, 2024

View reviewed changes

bsipocz merged commit 1f3c12f into astropy:main Jan 26, 2024
7 of 8 checks passed

ManonMarchand deleted the feature_Simbad_query_TAP branch January 29, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature simbad query tap #2856

Feature simbad query tap #2856

ManonMarchand commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 19, 2023 •

edited

Loading

ManonMarchand commented Oct 19, 2023

pep8speaks commented Oct 19, 2023 •

edited

Loading

ManonMarchand commented Oct 19, 2023

bsipocz commented Oct 19, 2023

ManonMarchand commented Oct 20, 2023 •

edited

Loading

bsipocz left a comment

ManonMarchand commented Nov 9, 2023 •

edited

Loading

bsipocz commented Nov 9, 2023

ManonMarchand commented Nov 9, 2023

bsipocz commented Nov 9, 2023

ManonMarchand commented Nov 10, 2023 •

edited

Loading

bsipocz commented Nov 10, 2023

ManonMarchand commented Nov 13, 2023

keflavich left a comment

ManonMarchand commented Dec 18, 2023

ManonMarchand commented Jan 26, 2024 •

edited

Loading

bsipocz left a comment

Feature simbad query tap #2856

Feature simbad query tap #2856

Conversation

ManonMarchand commented Oct 19, 2023 • edited Loading

What this PR does

Documentation-wise

Next steps

codecov bot commented Oct 19, 2023 • edited Loading

Codecov Report

ManonMarchand commented Oct 19, 2023

pep8speaks commented Oct 19, 2023 • edited Loading

Comment last updated at 2024-01-26 16:41:53 UTC

ManonMarchand commented Oct 19, 2023

bsipocz commented Oct 19, 2023

ManonMarchand commented Oct 20, 2023 • edited Loading

bsipocz left a comment

Choose a reason for hiding this comment

ManonMarchand commented Nov 9, 2023 • edited Loading

bsipocz commented Nov 9, 2023

ManonMarchand commented Nov 9, 2023

bsipocz commented Nov 9, 2023

ManonMarchand commented Nov 10, 2023 • edited Loading

bsipocz commented Nov 10, 2023

ManonMarchand commented Nov 13, 2023

keflavich left a comment

Choose a reason for hiding this comment

ManonMarchand commented Dec 18, 2023

ManonMarchand commented Jan 26, 2024 • edited Loading

bsipocz left a comment

Choose a reason for hiding this comment

ManonMarchand commented Oct 19, 2023 •

edited

Loading

codecov bot commented Oct 19, 2023 •

edited

Loading

pep8speaks commented Oct 19, 2023 •

edited

Loading

ManonMarchand commented Oct 20, 2023 •

edited

Loading

ManonMarchand commented Nov 9, 2023 •

edited

Loading

ManonMarchand commented Nov 10, 2023 •

edited

Loading

ManonMarchand commented Jan 26, 2024 •

edited

Loading