Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to `name` over ID attributes of columns as the names of columns in the `astropy.table.Table` instance. #2967

cosmoJFH · 2024-03-15T19:00:21Z

In issue #2911 it was shown that the function Gaia.launch_job returns the upper case column name DESIGNATION.

This came from that fact that IDs are used instead of the names in the votable at the time the astropy.table.Table is built.

Recently, the votables returned by the gaia archive has been updated in order to include the datalink "Service Descriptors", and they contain a few new IDs, where their corresponding values are capitalized. Therefore, this implies that the column names in the astropy.table.Table instances will be changed: they would be shown as uppercase names. This is an example:

>>> job = gaia.launch_job("select top 1 solution_id,source_id,designation from gaiadr3.gaia_source order by source_id")
>>> print(job.get_results())
    solution_id          SOURCE_ID              DESIGNATION         
------------------- ------------------- ----------------------------
1636148068921376768 4111834567779557376 Gaia DR3 4111834567779557376

We need to build astropy tables that contains column names based on the names and not on the IDs.

We have defined a new constant USE_NAMES_OVER_IDS=true in the GaiaClass. Its meaning is the same as in https://docs.astropy.org/en/stable/io/votable/index.html. The changes do not affect those projects that don't need to change the original built of the astropy.table.Table instances where IDs are used instead of names.

In this PR we have also fixed the function search_async_jobs in the class TapPlus. Making use of the present implementation an error is raised when it is used with a filter:

from astroquery.gaia import GaiaClass
>>> import astroquery as astroquery
>>> astroquery.__version__
'0.4.7'

from astroquery.utils.tap.model.filter import Filter

jobfilter = Filter()
jobfilter.limit=10


jobs = gaia.search_async_jobs(jobfilter=jobfilter, verbose=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jfernandez/anaconda3/lib/python3.10/site-packages/astroquery/utils/tap/core.py", line 1337, in search_async_jobs
    data = jobfilter.createUrlRequest()
AttributeError: 'Filter' object has no attribute 'createUrlRequest'

New tests have been made.

cc @esdc-esac-esa-int

Jira: GAIAPCR-1313

pep8speaks · 2024-03-15T19:00:31Z

Hello @cosmoJFH! 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-05-01 08:28:06 UTC

codecov · 2024-03-15T19:04:28Z

Codecov Report

Attention: Patch coverage is 53.94737% with 70 lines in your changes are missing coverage. Please review.

Project coverage is 66.85%. Comparing base (886817e) to head (c2fb611).
Report is 61 commits behind head on main.

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

Files	Patch %	Lines
astroquery/utils/tap/core.py	46.01%	61 Missing ⚠️
astroquery/gaia/core.py	75.00%	5 Missing ⚠️
astroquery/utils/tap/model/job.py	71.42%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2967      +/-   ##
==========================================
+ Coverage   66.83%   66.85%   +0.02%     
==========================================
  Files         237      237              
  Lines       18327    18304      -23     
==========================================
- Hits        12248    12237      -11     
+ Misses       6079     6067      -12

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

hectorcanovas · 2024-04-11T09:14:02Z

Dear Astropy/Astroquery team: From the message above ("Some checks were not successful") we see that there issues to be solved, but we do not understand if we (ESA/ESDC) should take care of them, or if the issues will be fixed by you (Astropy/Astroquery team) - thanks in advance for your answer.

keflavich · 2024-04-11T12:03:02Z

@hectorcanovas In general, these are items you should deal with if they are related to changes you made. Ideally, you should only see messages related to those.

However, we often have problems that crop up that are related to changes in upstream packages that do not indicate an issue with changes you've made. That applies in this case. The warnings on readthedocs are:

/home/docs/checkouts/readthedocs.org/user_builds/astroquery/envs/2967/lib/python3.10/site-packages/astroquery/alma/core.py:docstring of astroquery.alma.core.AlmaClass.query_sia:25: WARNING: py:obj reference target not found: pyvo.dam.obscore.POLARIZATION_STATES
/home/docs/checkouts/readthedocs.org/user_builds/astroquery/envs/2967/lib/python3.10/site-packages/astroquery/alma/core.py:docstring of astroquery.alma.core.AlmaClass.query_sia:66: WARNING: py:obj reference target not found: pyvo.dam.obscore.CALIBRATION_LEVELS
/home/docs/checkouts/readthedocs.org/user_builds/astroquery/envs/2967/lib/python3.10/site-packages/astroquery/ipac/irsa/core.py:docstring of astroquery.ipac.irsa.core.IrsaClass.query_sia:25: WARNING: py:obj reference target not found: pyvo.dam.obscore.POLARIZATION_STATES
/home/docs/checkouts/readthedocs.org/user_builds/astroquery/envs/2967/lib/python3.10/site-packages/astroquery/ipac/irsa/core.py:docstring of astroquery.ipac.irsa.core.IrsaClass.query_sia:66: WARNING: py:obj reference target not found: pyvo.dam.obscore.CALIBRATION_LEVELS

i.e., none of them are related to changes you made here.

The code coverage, on the other hand, is related to your changes:
https://github.com/astropy/astroquery/pull/2967/checks?check_run_id=22802134007
It shows that you've added several lines that are not covered by any tests. We'd prefer that you write tests to cover this new code, but in some cases (and I haven't reviewed so I'm not sure this is one of them), we decide the tests are too difficult to write or just not worth the effort, and say it's OK to merge anyway.

keflavich · 2024-04-11T12:05:03Z

Alright, after a quick review, the main changes that codecov is hitting are in TAP, which we aren't too worried about. @bsipocz has final say here, though.

cosmoJFH · 2024-04-27T10:51:49Z

Fixed the conflicts in the file CHANGE.rst. All checks passed.

…ed on names instead of IDs

… the function search_async_jobs. Update function __appendData that is called by search_async_jobs

…iables was removed

bsipocz

I have rebased this to pick up the recently merged changes, and in fact that highlighted a new regression.

I added a quick patch for it, so tests now pass but also see the comment. I'm happy going ahead with the merge as is, or if you want to double check the if all cases are covered in the conditional at the comment.

bsipocz · 2024-05-01T08:30:37Z

astroquery/utils/tap/xmlparser/utils.py

+
+        elif output_format == 'ecsv':
            result = APTable.read(data, format=astropy_format)
+        else:
+            result = APTable.read(data, format=astropy_format, use_names_over_ids=use_names_over_ids)


The handling of the ecsv case needed to be added due to the merging of #2983, however this made me wonder whether it's not the votable case that should be handled as special and fall back on not specifying use_names_over_ids to handle fits and ecsv, etc.

Thanks @bsipocz for your time reviewing the PR. I agree with you that the parameter use_names_over_ids must be used only for the format votable. We have updated the source code and include more tests in test_xmlparser.py to test this fact.

…otable

bsipocz

All looks good, and I added a fix for the windows int issue and rebased. Merging if all CI jobs come back passing.

bsipocz · 2024-05-03T02:20:04Z

Thank you @cosmoJFH!

bsipocz · 2024-05-03T07:04:44Z

I've pushed a dev version to pypi that includes this fix, so pip install -U --pre astroquery should be able to pick it up.

cosmoJFH · 2024-05-03T08:06:49Z

Thank you!! We really appreciate it.

cosmoJFH mentioned this pull request Mar 15, 2024

Gaia: get votable column names as lowercase names #2965

Closed

bsipocz added the gaia label Mar 18, 2024

bsipocz added this to the v0.4.8 milestone Mar 18, 2024

bsipocz added the utils.tap label Mar 18, 2024

cosmoJFH mentioned this pull request Apr 20, 2024

Gaia (and Tap/TapPlus?) .search_async_jobs() broken #2458

Open

Jorge Fernandez Hernandez and others added 16 commits April 29, 2024 19:12

GAIAPCR-1313 Update gaia astroquery to read votable columns names bas…

a4dde9c

…ed on names instead of IDs

GAIAPCR-1313 Update gaia astroquery to read votable columns names bas…

61eb9e6

…ed on names instead of IDs

GAIAPCR-1313 Update gaia astroquery to read votable columns names bas…

2138134

…ed on names instead of IDs

GAIAPCR-1313 Move function load_async_job to class TAP to make use of…

66aea1c

… the function search_async_jobs. Update function __appendData that is called by search_async_jobs

GAIAPCR-1313 Simplify function get_current_column_values_for_update

543fda9

GAIAPCR-1313 Format file

ce729fa

GAIAPCR-1313 Format file

50ee8fb

GAIAPCR-1313 Revert changes where the label _TapPlus_ for private var…

46c27a6

…iables was removed

GAIAPCR-1313 New tests

20fa044

GAIAPCR-1313 New remote test to check the funcntion search_async_jobs

03cb37f

GAIAPCR-1313 Reformat file

6fead2c

GAIAPCR-1313 Move function search_async_jobs to class Tap

74f7582

GAIAPCR-1313 Update documentation and include info in the file CHANGES

173fdcf

GAIAPCR-1313 Update PR number

1d9c914

GAIAPCR-1313 Fix PEP 8 issues

1db2786

Fixing ecsv case

852e621

bsipocz force-pushed the ESA_gaia_read_votables_columns_based_on_names_GAIAPCR-1313 branch from 33e3d32 to 852e621 Compare May 1, 2024 08:27

bsipocz approved these changes May 1, 2024

View reviewed changes

Jorge Fernandez Hernandez added 4 commits May 1, 2024 13:33

GAIAPCR-1313 the use_names_over_ids is only used for votable

2fc6daa

GAIAPCR-1313 Fix Code style checks

2f428cc

GAIAPCR-1313 Include new file extensions

b71c058

GAIAPCR-1313 Make use of astropy_format to check if the format is a v…

4422b7e

…otable

bsipocz force-pushed the ESA_gaia_read_votables_columns_based_on_names_GAIAPCR-1313 branch from 13c8ba6 to 48f272e Compare May 3, 2024 01:38

bsipocz approved these changes May 3, 2024

View reviewed changes

bsipocz force-pushed the ESA_gaia_read_votables_columns_based_on_names_GAIAPCR-1313 branch from 48f272e to 6be10bb Compare May 3, 2024 01:45

MAINT: fix int32 default on windows issue

65a00ad

bsipocz force-pushed the ESA_gaia_read_votables_columns_based_on_names_GAIAPCR-1313 branch from 6be10bb to 65a00ad Compare May 3, 2024 02:01

bsipocz merged commit d77dad2 into astropy:main May 3, 2024
8 checks passed

cosmoJFH mentioned this pull request Dec 17, 2024

fix: use the real column names rather than the IDs that astropy generates #3153

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to `name` over ID attributes of columns as the names of columns in the `astropy.table.Table` instance. #2967

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to `name` over ID attributes of columns as the names of columns in the `astropy.table.Table` instance. #2967

cosmoJFH commented Mar 15, 2024 •

edited

Loading

pep8speaks commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

hectorcanovas commented Apr 11, 2024

keflavich commented Apr 11, 2024

keflavich commented Apr 11, 2024

cosmoJFH commented Apr 27, 2024 •

edited

Loading

bsipocz left a comment

bsipocz May 1, 2024

cosmoJFH May 1, 2024

bsipocz left a comment

bsipocz commented May 3, 2024

bsipocz commented May 3, 2024

cosmoJFH commented May 3, 2024

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to name over ID attributes of columns as the names of columns in the astropy.table.Table instance. #2967

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to name over ID attributes of columns as the names of columns in the astropy.table.Table instance. #2967

Conversation

cosmoJFH commented Mar 15, 2024 • edited Loading

pep8speaks commented Mar 15, 2024 • edited Loading

Comment last updated at 2024-05-01 08:28:06 UTC

codecov bot commented Mar 15, 2024 • edited Loading

Codecov Report

hectorcanovas commented Apr 11, 2024

keflavich commented Apr 11, 2024

keflavich commented Apr 11, 2024

cosmoJFH commented Apr 27, 2024 • edited Loading

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz May 1, 2024

Choose a reason for hiding this comment

cosmoJFH May 1, 2024

Choose a reason for hiding this comment

bsipocz left a comment

Choose a reason for hiding this comment

bsipocz commented May 3, 2024

bsipocz commented May 3, 2024

cosmoJFH commented May 3, 2024

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to `name` over ID attributes of columns as the names of columns in the `astropy.table.Table` instance. #2967

Gaia: Define new constant USE_NAMES_OVER_IDS that gives preference to `name` over ID attributes of columns as the names of columns in the `astropy.table.Table` instance. #2967

cosmoJFH commented Mar 15, 2024 •

edited

Loading

pep8speaks commented Mar 15, 2024 •

edited

Loading

codecov bot commented Mar 15, 2024 •

edited

Loading

cosmoJFH commented Apr 27, 2024 •

edited

Loading