astroquery icon indicating copy to clipboard operation
astroquery copied to clipboard

Simbad: refactor to use TAP

Open ManonMarchand opened this issue 1 year ago • 9 comments

Hi astroquery,

[draft] Docs are not entirely clean yet, but the code should stay more or less like this before I un-draft. Here are some changes and questions about them:

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 does not exist anymore, but is replaced by a new criteria argument in every query_*** method (except query_tap because people can directly write that into their ADQL string).

This new interface looks like this:

from astroquery.simbad import Simbad

Simbad.query_region("NGC 3540", radius="1d", criteria="otype != 'Galaxy..'")
<Table length=568>
            main_id                     ra                dec         coo_err_maj coo_err_min coo_err_angle coo_wavelength     coo_bibcode    
                                       deg                deg             mas         mas          deg                                        
             object                  float64            float64         float32     float32       int16          str1             object      
------------------------------- ------------------ ------------------ ----------- ----------- ------------- -------------- -------------------
    Gaia DR3 762197128415141632 166.97067113513333  36.64013738050528      0.0392      0.0617            90              O 2020yCat.1350....0G
    Gaia DR3 761945928663296128  168.0368192975804  36.19277340338082      0.0493      0.0672            90              O 2020yCat.1350....0G
                    GRB 230606A           166.8095 36.484249999999996      2000.0      2000.0            --              X url:SWIFT          
                        CLS  37     168.2657239642     35.90652522302      0.8057      0.9704            90              O 2020yCat.1350....0G
         FIRST J110854.7+355104 167.22791666666666 35.851277777777774       570.0       510.0             0                1995ApJ...450..559B
         FIRST J110740.1+364843 166.91745833333334  36.81213888888889      1090.0      1090.0            90                1995ApJ...450..559B
                 [TKK2018] 3605          167.24513           37.00419          --          --            --              O 2018A&A...618A..81T
                 [TKK2018] 3621          167.66022           36.88636          --          --            --              O 2018A&A...618A..81T
            [T2015] nest 103154          167.92918           35.41882          --          --            --              O 2016A&A...588A..14T
    Gaia DR3 762127451160945024 167.38398588803042  36.33682265764583      0.0545      0.0578            90              O 2020yCat.1350....0G
...

Where the criteria string can be translated from the old syntax to the new one with the helper class:

from astroquery.simbad.utils import CriteriaTranslator
CriteriaTranslator.parse("region(GAL,180 0,2d) & otype = 'G' & (galdim_minaxis >= 10|galdim_majaxis >= 10)")
"CONTAINS(POINT('ICRS', ra, dec), CIRCLE('ICRS', 86.40498828654475, 28.93617776179148, 2.0)) = 1 AND otype = 'G' AND (galdim_minaxis >= 10 OR galdim_majaxis >= 10)"

This could also be done automatically at each criteria insertion (like detecting if this is the old or new format and translating, maybe with a warning indicating the new ADQL syntax?). What are your thoughts?

On the output

Adding columns to the output

This was done with add_votable_fields before. This is replaced by add_to_output where the arguments are reproduced to fit as close as possible to the votable fields, but some were documented here but deprecated for years in Simbad. Seeing that there is no issue opened in this repo, these are not used.

That'd look like this:

  • someone calling a votable field deprecated for years:
from astroquery.simbad import Simbad
simbad = Simbad()
simbad.add_to_output("einstein")
ValueError: 'einstein' is no longer a part of SIMBAD. It was moved into a separate VizieR catalog. It is possible to query it with the `astroquery.vizier` module.
  • someone calling with a column/table that has a different name between the old and new interfaces
from astroquery.simbad import Simbad
simbad = Simbad()
simbad.add_to_output("id(1)")
<stdin>:1: DeprecationWarning: 'id(1)' has been renamed 'main_id'. You'll see it appearing with its new name in the output table

Every other possibility can be listed with:

from astroquery.simbad import Simbad
Simbad.list_output_options()
<Table length=97>
       name                                       description                                            type         
      object                                         object                                             object        
----------------- ---------------------------------------------------------------------------- -----------------------
              ids                                             all names concatenated with pipe                   table
         otypedef                               all names and definitions for the object types                   table
            ident                                        Identifiers of an astronomical object                   table
             flux                      Magnitude/Flux information about an astronomical object                   table
        allfluxes                             all flux/magnitudes U,B,V,I,J,H,K,u_,g_,r_,i_,z_                   table
          has_ref Associations between astronomical objects and their bibliographic references                   table
      mesDistance                   Collection of distances (pc, kpc or Mpc) by several means.                   table
...

(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 :slightly_smiling_face: )

Changes in output style

  • As hinted above, some columns don't have the same name in the two Simbad interfaces. They are very close though.
  • Unfortunately, the votable output of Simbad-TAP has lower case column names. This is a breaking change from previous interface.
  • The default output format for coordinates is degrees rather than sexadecimal like before.

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, see later is this long PR description.

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 in query_object is replaced by matched_id that contains the ID that corresponded to the wildcard expression.

It looks like this:

from astroquery.simbad import Simbad
simbad = Simbad()
simbad.add_to_output("otype")
simbad.ROW_LIMIT = 20
eso_clusters = simbad.query_object("ESO*", wildcard=True, criteria="otype='Cl*..'")
eso_clusters["main_id", "ra", "dec", "otype", "matched_id"]
<Table length=20>
     main_id               ra                 dec         otype   matched_id
                          deg                 deg                           
      object            float64             float64       object    object  
------------------ ------------------ ------------------- ------ -----------
         NGC  1776           74.66475  -66.42954166666667    Cl*  ESO  85-28
        ESO  57-81   96.2041666666667  -71.65833333333335    Cl*  ESO  57-81
         NGC  2097           86.02948           -62.78351    Cl*  ESO  86-28
         NGC  1865  78.10391666666668  -68.77200277777777    Cl*  ESO  56-78
         NGC   299 13.353083333333332  -72.19655555555556    Cl*   ESO  51-5
         NGC  1849  77.39450000000001  -66.31465833333334    Cl*  ESO  85-49
         NGC  1826  76.39174999999999  -66.22904166666666    Cl*  ESO  85-43
        ESO  85-41           76.35151           -63.28754    Cl*  ESO  85-41
         NGC  2121  87.05495833333332  -71.48111111111112    Cl*  ESO  57-40
         NGC  2019  82.98533333333333  -70.15902777777778    Cl* ESO  56-145
         NGC  1777              73.95  -74.28333333333333    Cl*   ESO  33-1
         NGC  2047  83.98316666666666  -70.18519722222223    Cl* ESO  56-167
Cl Westerlund    1             251.76 -45.852000000000004    Cl*  ESO 277-12
    Cl Terzan    5 267.02083333333337 -24.779999999999998    Cl*  ESO 520-27
         ESO  51-3 12.208333333333334  -69.86999999999999    Cl*   ESO  51-3
         NGC  2257  97.55258333333333  -64.32777777777777    Cl*  ESO  87-24
         NGC  2203  91.17570833333333  -75.43772222222222    Cl*   ESO  34-4
         NGC  1751  73.55379166666667  -69.80744444444444    Cl*  ESO  56-23
         NGC   416 16.995958333333334  -72.35569444444444    Cl*  ESO  29-32
         NGC   411  16.98183333333333  -71.76752777777777    Cl*  ESO  51-19

Query_objects

It now has a typed_id column as requested in #967 . The object_number_id replaces script_number_id (but this could be reverted, it's just strange as there is really one script)

Looks like this:

from astroquery.simbad import Simbad
Simbad.query_objects(("m1", "m2", "m503", "m21"))
<Table length=4>
main_id         ra                 dec         coo_err_maj coo_err_min coo_err_angle coo_wavelength     coo_bibcode     typed_id object_number_id
               deg                 deg             mas         mas          deg                                                                  
 object      float64             float64         float32     float32       int16          str1             object        object       int64      
------- ------------------ ------------------- ----------- ----------- ------------- -------------- ------------------- -------- ----------------
  M   1            83.6287             22.0147     18500.0     18500.0             0              R 1995AuJPh..48..143S       m1                1
  M   2 323.36258333333336 -0.8232499999999998          --          --            --              O 2010AJ....140.1830G       m2                2
                        --                  --          --          --            --                                        m503                3
  M  21            271.036 -22.505000000000003          --          --            --              O 2021A&A...647A..19T      m21                4

Note that the requested feature (I could not find the issue anymore) 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:

from astroquery.simbad import Simbad
Simbad.query_bibcode("1894MNRAS..55...17L")
<Table length=1>
                                                               References                                                              
                                                                 str132                                                                
---------------------------------------------------------------------------------------------------------------------------------------
1894MNRAS..55...17L = DOI 10.1093/mnras/55.1.17\nMon. Not. R. Astron. Soc., 55, 17 (1894)\nLEWIS T.\nNote on the orbit of kappa Pegasi.

New output:

from astroquery.simbad import Simbad
Simbad.query_bibcode("1894MNRAS..55...17L")
<Table length=1>
      bibcode                doi          journal nbobject  page last_page               title                volume  year
       object               object         object  int32   int32   int32                 object               int32  int16
------------------- --------------------- ------- -------- ----- --------- ---------------------------------- ------ -----
1894MNRAS..55...17L 10.1093/mnras/55.1.17   MNRAS        1    17        -- Note on the orbit of kappa Pegasi.     55  1894

I know it's a very different output but I really hated the former one. I can try to reproduce the old one but :frowning_face:

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)

ManonMarchand avatar Feb 22 '24 09:02 ManonMarchand

Hello @ManonMarchand! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 13:12: E221 multiple spaces before operator Line 14:11: E221 multiple spaces before operator Line 15:12: E221 multiple spaces before operator Line 16:13: E221 multiple spaces before operator Line 18:12: E221 multiple spaces before operator Line 18:121: E501 line too long (498 > 120 characters)

Line 20:121: E501 line too long (458 > 120 characters) Line 21:1: W293 blank line contains whitespace Line 22:24: E231 missing whitespace after ':' Line 22:28: E231 missing whitespace after ',' Line 22:30: E231 missing whitespace after ',' Line 22:32: E231 missing whitespace after ',' Line 22:34: E231 missing whitespace after ',' Line 22:36: E231 missing whitespace after ',' Line 22:39: E231 missing whitespace after ',' Line 22:41: E231 missing whitespace after ',' Line 22:43: E231 missing whitespace after ',' Line 22:45: E231 missing whitespace after ',' Line 22:48: E231 missing whitespace after ',' Line 22:57: E231 missing whitespace after ':' Line 22:61: E231 missing whitespace after ',' Line 22:63: E231 missing whitespace after ',' Line 22:65: E231 missing whitespace after ',' Line 22:67: E231 missing whitespace after ',' Line 22:69: E231 missing whitespace after ',' Line 22:71: E231 missing whitespace after ',' Line 22:74: E231 missing whitespace after ',' Line 22:76: E231 missing whitespace after ',' Line 22:78: E231 missing whitespace after ',' Line 22:80: E231 missing whitespace after ',' Line 22:83: E231 missing whitespace after ',' Line 22:86: E231 missing whitespace after ',' Line 22:95: E231 missing whitespace after ':' Line 22:99: E231 missing whitespace after ',' Line 22:101: E231 missing whitespace after ',' Line 22:103: E231 missing whitespace after ',' Line 22:105: E231 missing whitespace after ',' Line 22:107: E231 missing whitespace after ',' Line 22:110: E231 missing whitespace after ',' Line 22:112: E231 missing whitespace after ',' Line 22:114: E231 missing whitespace after ',' Line 22:116: E231 missing whitespace after ',' Line 22:119: E231 missing whitespace after ',' Line 22:121: E501 line too long (598 > 120 characters) Line 22:126: E231 missing whitespace after ':' Line 22:130: E231 missing whitespace after ',' Line 22:132: E231 missing whitespace after ',' Line 22:135: E231 missing whitespace after ',' Line 22:138: E231 missing whitespace after ',' Line 22:141: E231 missing whitespace after ',' Line 22:144: E231 missing whitespace after ',' Line 22:147: E231 missing whitespace after ',' Line 22:150: E231 missing whitespace after ',' Line 22:153: E231 missing whitespace after ',' Line 22:156: E231 missing whitespace after ',' Line 22:159: E231 missing whitespace after ',' Line 22:161: E231 missing whitespace after ',' Line 22:164: E231 missing whitespace after ',' Line 22:168: E231 missing whitespace after ',' Line 22:171: E231 missing whitespace after ',' Line 22:174: E231 missing whitespace after ',' Line 22:177: E231 missing whitespace after ',' Line 22:180: E231 missing whitespace after ',' Line 22:183: E231 missing whitespace after ',' Line 22:186: E231 missing whitespace after ',' Line 22:189: E231 missing whitespace after ',' Line 22:192: E231 missing whitespace after ',' Line 22:195: E231 missing whitespace after ',' Line 22:198: E231 missing whitespace after ',' Line 22:202: E231 missing whitespace after ':' Line 22:206: E231 missing whitespace after ',' Line 22:208: E231 missing whitespace after ',' Line 22:210: E231 missing whitespace after ',' Line 22:213: E231 missing whitespace after ',' Line 22:216: E231 missing whitespace after ',' Line 22:219: E231 missing whitespace after ',' Line 22:222: E231 missing whitespace after ',' Line 22:225: E231 missing whitespace after ',' Line 22:228: E231 missing whitespace after ',' Line 22:231: E231 missing whitespace after ',' Line 22:234: E231 missing whitespace after ',' Line 22:237: E231 missing whitespace after ',' Line 22:239: E231 missing whitespace after ',' Line 22:242: E231 missing whitespace after ',' Line 22:246: E231 missing whitespace after ',' Line 22:248: E231 missing whitespace after ',' Line 22:250: E231 missing whitespace after ',' Line 22:252: E231 missing whitespace after ',' Line 22:255: E231 missing whitespace after ',' Line 22:258: E231 missing whitespace after ',' Line 22:261: E231 missing whitespace after ',' Line 22:264: E231 missing whitespace after ',' Line 22:267: E231 missing whitespace after ',' Line 22:270: E231 missing whitespace after ',' Line 22:273: E231 missing whitespace after ',' Line 22:276: E231 missing whitespace after ',' Line 22:280: E231 missing whitespace after ':' Line 22:284: E231 missing whitespace after ',' Line 22:286: E231 missing whitespace after ',' Line 22:288: E231 missing whitespace after ',' Line 22:291: E231 missing whitespace after ',' Line 22:294: E231 missing whitespace after ',' Line 22:297: E231 missing whitespace after ',' Line 22:300: E231 missing whitespace after ',' Line 22:303: E231 missing whitespace after ',' Line 22:306: E231 missing whitespace after ',' Line 22:309: E231 missing whitespace after ',' Line 22:312: E231 missing whitespace after ',' Line 22:315: E231 missing whitespace after ',' Line 22:317: E231 missing whitespace after ',' Line 22:320: E231 missing whitespace after ',' Line 22:324: E231 missing whitespace after ',' Line 22:326: E231 missing whitespace after ',' Line 22:328: E231 missing whitespace after ',' Line 22:330: E231 missing whitespace after ',' Line 22:333: E231 missing whitespace after ',' Line 22:336: E231 missing whitespace after ',' Line 22:339: E231 missing whitespace after ',' Line 22:342: E231 missing whitespace after ',' Line 22:345: E231 missing whitespace after ',' Line 22:348: E231 missing whitespace after ',' Line 22:351: E231 missing whitespace after ',' Line 22:354: E231 missing whitespace after ',' Line 22:372: E231 missing whitespace after ':' Line 22:376: E231 missing whitespace after ',' Line 22:378: E231 missing whitespace after ',' Line 22:381: E231 missing whitespace after ',' Line 22:384: E231 missing whitespace after ',' Line 22:389: E231 missing whitespace after ':' Line 22:393: E231 missing whitespace after ',' Line 22:395: E231 missing whitespace after ',' Line 22:398: E231 missing whitespace after ',' Line 22:401: E231 missing whitespace after ',' Line 22:408: E231 missing whitespace after ':' Line 22:412: E231 missing whitespace after ',' Line 22:414: E231 missing whitespace after ',' Line 22:418: E231 missing whitespace after ',' Line 22:421: E231 missing whitespace after ',' Line 22:431: E231 missing whitespace after ':' Line 22:435: E231 missing whitespace after ',' Line 22:437: E231 missing whitespace after ',' Line 22:441: E231 missing whitespace after ',' Line 22:444: E231 missing whitespace after ',' Line 22:448: E231 missing whitespace after ':' Line 22:452: E231 missing whitespace after ',' Line 22:454: E231 missing whitespace after ',' Line 22:457: E231 missing whitespace after ',' Line 22:460: E231 missing whitespace after ',' Line 22:463: E231 missing whitespace after ',' Line 22:466: E231 missing whitespace after ',' Line 22:469: E231 missing whitespace after ',' Line 22:472: E231 missing whitespace after ',' Line 22:475: E231 missing whitespace after ',' Line 22:478: E231 missing whitespace after ',' Line 22:481: E231 missing whitespace after ',' Line 22:483: E231 missing whitespace after ',' Line 22:488: E231 missing whitespace after ',' Line 22:491: E231 missing whitespace after ',' Line 22:494: E231 missing whitespace after ',' Line 22:497: E231 missing whitespace after ',' Line 22:500: E231 missing whitespace after ',' Line 22:503: E231 missing whitespace after ',' Line 22:506: E231 missing whitespace after ',' Line 22:509: E231 missing whitespace after ',' Line 22:512: E231 missing whitespace after ',' Line 22:515: E231 missing whitespace after ',' Line 22:518: E231 missing whitespace after ',' Line 22:521: E231 missing whitespace after ',' Line 22:530: E231 missing whitespace after ':' Line 22:534: E231 missing whitespace after ',' Line 22:537: E231 missing whitespace after ',' Line 22:540: E231 missing whitespace after ',' Line 22:542: E231 missing whitespace after ',' Line 22:546: E231 missing whitespace after ',' Line 22:549: E231 missing whitespace after ',' Line 22:552: E231 missing whitespace after ',' Line 22:555: E231 missing whitespace after ',' Line 22:564: E231 missing whitespace after ':' Line 22:568: E231 missing whitespace after ',' Line 22:570: E231 missing whitespace after ',' Line 22:574: E231 missing whitespace after ',' Line 22:577: E231 missing whitespace after ',' Line 22:584: E231 missing whitespace after ':' Line 22:588: E231 missing whitespace after ',' Line 22:590: E231 missing whitespace after ',' Line 22:594: E231 missing whitespace after ',' Line 22:597: E231 missing whitespace after ',' Line 26:4: E111 indentation is not a multiple of four Line 26:10: E231 missing whitespace after ',' Line 26:26: E231 missing whitespace after ',' Line 27:7: E111 indentation is not a multiple of four Line 27:10: E713 test for membership should be 'not in' Line 27:30: E701 multiple statements on one line (colon) Line 27:31: E241 multiple spaces after ':' Line 28:7: E111 indentation is not a multiple of four Line 31:29: E231 missing whitespace after ':' Line 31:33: E231 missing whitespace after ',' Line 31:35: E231 missing whitespace after ',' Line 31:37: E231 missing whitespace after ',' Line 31:39: E231 missing whitespace after ',' Line 31:41: E231 missing whitespace after ',' Line 31:44: E231 missing whitespace after ',' Line 31:46: E231 missing whitespace after ',' Line 31:49: E231 missing whitespace after ',' Line 31:52: E231 missing whitespace after ',' Line 31:55: E231 missing whitespace after ',' Line 35:4: E111 indentation is not a multiple of four Line 36:8: E111 indentation is not a multiple of four Line 36:11: E713 test for membership should be 'not in' Line 36:29: E701 multiple statements on one line (colon) Line 37:8: E111 indentation is not a multiple of four Line 40:3: E121 continuation line under-indented for hanging indent Line 40:20: E231 missing whitespace after ',' Line 40:25: E231 missing whitespace after ',' Line 40:27: E231 missing whitespace after ',' Line 40:32: E231 missing whitespace after ',' Line 40:37: E231 missing whitespace after ',' Line 41:37: E231 missing whitespace after ',' Line 41:48: E231 missing whitespace after ',' Line 41:50: E231 missing whitespace after ',' Line 41:66: E231 missing whitespace after ',' Line 41:77: E231 missing whitespace after ',' Line 42:37: E231 missing whitespace after ',' Line 42:48: E231 missing whitespace after ',' Line 42:50: E231 missing whitespace after ',' Line 42:67: E231 missing whitespace after ',' Line 42:78: E231 missing whitespace after ',' Line 43:30: E231 missing whitespace after ',' Line 43:41: E231 missing whitespace after ',' Line 43:43: E231 missing whitespace after ',' Line 43:68: E231 missing whitespace after ',' Line 43:79: E231 missing whitespace after ',' Line 44:47: E231 missing whitespace after ',' Line 44:58: E231 missing whitespace after ',' Line 44:60: E231 missing whitespace after ',' Line 44:80: E231 missing whitespace after ',' Line 44:91: E231 missing whitespace after ',' Line 45:47: E231 missing whitespace after ',' Line 45:58: E231 missing whitespace after ',' Line 45:60: E231 missing whitespace after ',' Line 45:80: E231 missing whitespace after ',' Line 45:91: E231 missing whitespace after ',' Line 46:32: E231 missing whitespace after ',' Line 46:43: E231 missing whitespace after ',' Line 46:45: E231 missing whitespace after ',' Line 46:65: E231 missing whitespace after ',' Line 46:76: E231 missing whitespace after ',' Line 47:47: E231 missing whitespace after ',' Line 47:58: E231 missing whitespace after ',' Line 47:60: E231 missing whitespace after ',' Line 47:89: E231 missing whitespace after ',' Line 47:100: E231 missing whitespace after ',' Line 48:36: E231 missing whitespace after ',' Line 48:47: E231 missing whitespace after ',' Line 48:49: E231 missing whitespace after ',' Line 48:67: E231 missing whitespace after ',' Line 48:78: E231 missing whitespace after ',' Line 49:39: E231 missing whitespace after ',' Line 49:50: E231 missing whitespace after ',' Line 49:52: E231 missing whitespace after ',' Line 49:73: E231 missing whitespace after ',' Line 49:84: E231 missing whitespace after ',' Line 50:24: E231 missing whitespace after ',' Line 50:35: E231 missing whitespace after ',' Line 50:37: E231 missing whitespace after ',' Line 50:57: E231 missing whitespace after ',' Line 50:68: E231 missing whitespace after ','

Comment last updated at 2024-06-20 22:18:08 UTC

pep8speaks avatar Feb 22 '24 09:02 pep8speaks

On codestyle:

tox -e codestyle tells me:

❯ tox -e codestyle
codestyle: commands[0]> flake8 astroquery --count
astroquery/simbad/tests/test_deprecated_simbad.py:482:78: W504 line break after binary operator

and here I should break before? Which one do you prefer? I'm happy with both.

Edit: I applied the line breaking after operator rule to reduce the number of issues with the PEP8 detector here.

On criteria_lextab.py and criteria_parser.py

These files are automatically generated by the astroquery.utils.parsing tools (lex and yacc) so I'd think I shouldn't edit them?

Similar files are not linted in astropy code base ex: https://github.com/astropy/astropy/blob/main/astropy/units/format/generic_parsetab.py

ManonMarchand avatar Feb 22 '24 13:02 ManonMarchand

Codecov Report

Attention: Patch coverage is 97.12526% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.52%. Comparing base (0ffd5d4) to head (98ae3b4). Report is 55 commits behind head on main.

:exclamation: Current head 98ae3b4 differs from pull request most recent head 6594ebb

Please upload reports for the commit 6594ebb to get more accurate results.

Files Patch % Lines
astroquery/simbad/core.py 95.95% 12 Missing :warning:
astroquery/simbad/setup_package.py 33.33% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2954      +/-   ##
==========================================
+ Coverage   67.17%   67.52%   +0.34%     
==========================================
  Files         231      233       +2     
  Lines       18247    18224      -23     
==========================================
+ Hits        12258    12305      +47     
+ Misses       5989     5919      -70     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 22 '24 16:02 codecov[bot]

On the oldest versions for all dependencies failure:

I thought yacc and lex just changed places between astropy 4.2.1 and astropy 4.3, but some method's signatures changed too, see commit here https://github.com/astropy/astropy/commit/85bffd9f0241bcbbc70c6b81c4b2281a5ed55bc6

The currently minimal astropy version supported in astroquery is 4.2.1. Is the jump negotiable? I don't see a lot of deprecations in the change log between these two versions.

ManonMarchand avatar Feb 23 '24 13:02 ManonMarchand

The currently minimal astropy version supported in astroquery is 4.2.1. Is the jump negotiable? I don't see a lot of deprecations in the change log between these two versions.

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.

bsipocz avatar Feb 23 '24 19:02 bsipocz

astroquery/simbad/tests/test_deprecated_simbad.py:482:78: W504 line break after binary operator and here I should break before? Which one do you prefer? I'm happy with both.

We enforce W504 and ignore W503 as break before operator is a bit more legible logic.

bsipocz avatar Feb 23 '24 19:02 bsipocz

Hello!

I'm almost in a un-draft state :)

Here are the remaining points that I'm unsure about:

  • which version of astroquery do you think this should be merged in? Is there a way to leave an extended changes description somewhere? Like a more narrative changelog?
  • what did I do wrong with the docs? I don't have failures locally (only warnings about other modules)

ManonMarchand avatar Mar 28 '24 16:03 ManonMarchand

Sorry for the delay in responding:

which version of astroquery do you think this should be merged in? Is there a way to leave an extended changes description somewhere? Like a more narrative changelog?

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. For a more detailed narrative about the changes I would suggest to add a section in the narrative documentation. In practice that is the place users will notice features, I doubt that many refer to the changelog to obtain extended descriptions.

what did I do wrong with the docs? I don't have failures locally (only warnings about other modules)

I suspect this also run into the same pyvo related RTD failure and was unrelated to your changes.

bsipocz avatar Apr 08 '24 21:04 bsipocz

Switching back to draft, I'm tweaking a bit the support of the legacy 'query_criteria'

ManonMarchand avatar Apr 16 '24 09:04 ManonMarchand

Hi all! Sorry for the long delay, this should be ready to review. The description on top of the PR is updated to this new state

ManonMarchand avatar Jun 03 '24 16:06 ManonMarchand

Sorry for the long delay, this should be ready to review. The description on top of the PR is updated to this new state

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.

bsipocz avatar Jun 04 '24 16:06 bsipocz

@keflavich : I think all your review comments are addressed? @bsipocz : no worries :slightly_smiling_face:

ManonMarchand avatar Jun 05 '24 13:06 ManonMarchand

I found out that the wildcards in query_objects did not work. This is fixed now, but the remote test is so slow that I marked it to be skipped with the SKIP_SLOW boolean, like what the Alma module does for its own slow tests.

I'm not sure that supporting the wildcards in this method is absolutely necessary, or maybe I don't get the use case. Anyhow, it works now for patient people.

ManonMarchand avatar Jun 14 '24 16:06 ManonMarchand

I think all your comments are addressed except the ones about parameters deprecation. What is the correct way of doing?

ManonMarchand avatar Jun 20 '24 15:06 ManonMarchand

I think all your comments are addressed except the ones about parameters deprecation. What is the correct way of doing?

As I see you addressed them (most was already done in the code), it's just the changelog that needs a bit of rephrasing.

bsipocz avatar Jun 20 '24 21:06 bsipocz