Skip to content

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Mar 4, 2019

Edit: to close #1777

As the reader for the csv format cannot handle column duplications (that is quite likely as both users and catalogues can name their RA/DEC columns as RA/DEC, and there is no renaming on the server side). Until astropy solves this for the ascii reader, and we require that version as minimum version we need to have a workaround.

Here is a failing example:

from astropy.table import Table
import astropy.units as u
from  astroquery.xmatch import XMatch

input_table=Table.read("""2.11044940645274 0.688137082702486
    2.11504231688281 0.671688214247088
    2.08882950663451 0.719216362471814
    2.09308610414763 0.708799922876203
    2.09619379588014 0.706120900363544""", names=['ra', 'dec'], format='ascii')

XMatch.query(input_table, 'vizier:II/311/wise', 5*u.arcsec, 'ra', 'dec')

The downside of this PR that the votable will remove also columns that are totally fine otherwise (e.g. _2MASS in the example in the current docs)

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #1375 (47d993e) into main (0cc58b7) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #1375      +/-   ##
==========================================
- Coverage   64.20%   64.19%   -0.01%     
==========================================
  Files         130      130              
  Lines       16892    16881      -11     
==========================================
- Hits        10845    10837       -8     
+ Misses       6047     6044       -3     
Impacted Files Coverage Δ
astroquery/xmatch/core.py 74.35% <50.00%> (+0.20%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@keflavich
Copy link
Contributor

If we know in advance which columns are going to be returned by xmatch, we can modify the user's input columns by adding a preceding user_ to each column that would be duplicated, or just to everything the user submits.

@bsipocz
Copy link
Member Author

bsipocz commented Mar 4, 2019

@keflavich - I felt that hacking around like that will be messier. Also, how would we know what are the columns? Do a dummy query? I would also prefer to change the column names in the remote catalog than in the user's given the latter was provided by the user so will be more less intuitive to get the names changed. Or you mean to change the name, upload and then hack back the names before returning the results to the user?

@keflavich
Copy link
Contributor

It's OK to have some of these hacks in astroquery. You're right that adding xmatch_ as the prefix to the returned columns would be more intuitive, though. I don't see any major drawbacks to that approach

@bsipocz
Copy link
Member Author

bsipocz commented Mar 4, 2019

OK, let's do that hack instead, I'll try to get back to it in the evening.

@fxpineau
Copy link
Contributor

fxpineau commented Mar 14, 2019

I am not sure it is properly documented, but you can know in advance the column names of the VizieR table you are xmatching using the following URL (example for WISE with a JSON output):
http://cdsxmatch.u-strasbg.fr/xmatch/api/v1/sync/tables?action=getColList&tabName=II/311/wise&responseFormat=json

@bsipocz
Copy link
Member Author

bsipocz commented Mar 14, 2019

Thanks @fxpineau for the info. I have a WIP hack locally, will see which approach provides a cleaner solution.

@bsipocz
Copy link
Member Author

bsipocz commented Sep 10, 2019

/rebase

@bsipocz
Copy link
Member Author

bsipocz commented Sep 10, 2019

there are some votable related discussions going on triggered by changes (and test failures) here, so I would simply postpone to finish this off in the next release.

(note: the bug this aimed to address has already been fixed, so this PR now is a pure enhancement only to switch to use a votable return rather than the current csv)

@bsipocz bsipocz modified the milestones: v0.3.10, 0.4.0 Sep 10, 2019
@bsipocz bsipocz modified the milestones: v0.4, v4 Jan 23, 2020
@bsipocz
Copy link
Member Author

bsipocz commented Nov 27, 2022

The snippet from above works now without this PR, but switching to VOTable based return has been raised since then (e.g. #1777), so it maybe still worth rebasing and fixing up this PR.

@bsipocz bsipocz force-pushed the xmatch_votable branch 2 times, most recently from e480200 to 3813328 Compare November 27, 2022 06:45
@bsipocz bsipocz requested a review from keflavich November 27, 2022 06:45
@bsipocz bsipocz modified the milestones: v4, v0.4.7 Nov 27, 2022
@bsipocz bsipocz merged commit 113148f into astropy:main Nov 27, 2022
@bsipocz bsipocz deleted the xmatch_votable branch April 9, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

XMatch: use votable return
3 participants