astroquery icon indicating copy to clipboard operation
astroquery copied to clipboard

Renaming the TAP method load_tables into load_tables_metadata

Open ManonMarchand opened this issue 2 years ago • 3 comments

Hi!

the issue

The load_table and load_tables methods from utils.tap don't load tables. It is a bit confusing, see the Data Carpentry teaching material here:

https://github.com/datacarpentry/astronomy-python/blob/7d0bf1c47e600a38bacf7ddd4c45ca2a786726a2/episodes/01-query.md?plain=1#L161

This is not written in the methods docstrings.

from astroquery.gaia import Gaia
help(Gaia.load_tables)

returns

Help on method load_tables in module astroquery.utils.tap.core:

load_tables(*, only_names=False, include_shared_tables=False, verbose=False) method of astroquery.gaia.core.GaiaClass instance
    Loads all public tables
    
    Parameters
    ----------
    only_names : bool, TAP+ only, optional, default 'False'
        True to load table names only
    include_shared_tables : bool, TAP+, optional, default 'False'
        True to include shared tables
    verbose : bool, optional, default 'False'
        flag to display information about the process
    
    Returns
    -------
    A list of table objects

(It also states that the returned value is a list of table objects while it returns a list of TapTableMeta (with only metadata).)

possible solutions

We could rename the methods load_tables and load_table into load_tables_metadata and load_table_metadata. If this is too much of an API change, then we rewrite the docstring?

I could open a PR if you think it's a good idea :slightly_smiling_face:

ManonMarchand avatar Oct 26 '23 18:10 ManonMarchand

At this point, I don't think renaming the method would be worth the deprecation. We have plenty of examples in the astroquery docs about its usage, and if data carpentry misunderstood it, it's beyond our remit (though extending the docstring is of course possible.)

https://astroquery.readthedocs.io/en/latest/gaia/gaia.html#getting-public-tables-metadata https://astroquery.readthedocs.io/en/latest/gaia/gaia.html#listing-shared-tables https://astroquery.readthedocs.io/en/latest/esa/jwst/jwst.html#getting-public-tables https://astroquery.readthedocs.io/en/latest/utils/tap.html

(FYI: Ideally, we would like to switch out utils.tap in favour of pyvo, so I would say any functional enhancement to utils.tap should have a low priority and we should instead try to help the switch. But if the ESA developers want to address this here, I would leave it to their judgment. )

cc @esdc-esac-esa-int

bsipocz avatar Oct 26 '23 21:10 bsipocz

Dear @ManonMarchand ,

Many thanks for your inputs. I agree with @bsipocz , it is not worthy to rename the method.

  • This will affect the rest of ESA modules, so we should test all of them.
  • On the other hand, maybe it would be a better idea to update the documentation, so the purpose of the method is clear.
  • Finally, yes, we should start moving to pyvo methods, so we are currently analyzing the implications and effort we need.

If it is ok for you if we update the documentation, please let us know and we will modify it and create the PR.

jespinosaar avatar Oct 27 '23 10:10 jespinosaar

Sure! Updating the documentation is good too

ManonMarchand avatar Oct 27 '23 12:10 ManonMarchand

Docu update has happened, so I'm closing this now. Feel free to reopen if you think more improvement is needed.

bsipocz avatar May 21 '24 01:05 bsipocz