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

[ManonMarchand]

Hello astroquery maintainers 🙂

This PR fixes #3124

The issue was not in the votable parser in astropy, but in the conversion from votable to astropy table where for vizier the option "use_names_overs_ids" should be preferred, as vizier does not write IDs for its fields but uses names instead.

The IDs that we saw in the issue report are generated by the votable parser when a field has no ID.

See MRE for this:

from astropy.io.votable.tree import VOTableFile, Resource, TableElement, Field
votable = VOTableFile()
table = TableElement(votable)
resource = Resource()
votable.resources.append(resource)
resource.tables.append(table)
table.fields.extend([Field(votable, name="filename(myname", datatype="char", arraysize="*")])

WARNING: W03: ?:?:?: W03: Implicitly generating an ID from a name 'filename(myname' -> 'filename_myname' [astropy.io.votable.xmlutil]

So switching from ID to name for the names of the columns in the conversion from votable to astropy table did the trick.

This is a bit of a breaking change, as people will start to see the real column names rather than the underscore ones. It also appeared from the narrative docs that this strange option was adding some underscores at the beginning of some column names event when no unconventional characters are present.

Edit: the underscore at the beginning is added when the column name starts with a letter... something forbidden by the standard that vizier still does 😰 (and I don't think we can change this upstream)

Edit bis: no, starting with a number is valid. I was confused by the text of the standard, but from reading the xsd at the bottom, the name should be of type xsd:token, which is defined here: https://www.w3.org/TR/xmlschema11-2/#token
Looks like the xsd:token can be 2MASS without issues 🙂

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.54%. Comparing base (04c6069) to head (04fcc57).
Report is 13 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3153   +/-   ##
=======================================
  Coverage   67.54%   67.54%           
=======================================
  Files         233      233           
  Lines       18493    18493           
=======================================
  Hits        12491    12491           
  Misses       6002     6002           

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

[keflavich]

@ManonMarchand you mean if the column started with a number, an underscore was added, right?

This PR looks good to me.

I think we should have a little discussion about the breaking nature of the PR - do we need to have an option to support the old column naming scheme? IMO, no, let's fix the bug and move on, but as a heavy user of Vizier, I have relied on its stability in many places in my own software, and a sudden column name change may require substantial refactoring.

Could you add a Note to the narrative documentation indicating what this change was, why it happened, and (roughly) when it did - i.e., astroquery 0.4.8 / late 2024? We don't do this in all cases, but given the subtlety of the change and the long-standing nature of the issue in a broadly-used package, it is warranted.

[ManonMarchand]

Yes, that's exactly what I mean.

Sure I can write a note. I'm also a bit worried about this breaking change (still a fix, though) but I'm also not in favor of an option to keep the old behavior.

[cosmoJFH]

Just for the records, a similar change was done in the gaia module: #2967

[bsipocz]

If Adam is happy with getting this change in from the user's POV and think of it more as a bugfix than a behaviour change, then I'm also OK with merging it now.

[keflavich]

Yes, I'm happy with it - but give @ManonMarchand a chance to evaluate my suggested docs rephrasing before we merge.

[bsipocz]

Thank you both!