fix: use the real column names rather than the IDs that astropy generates#3153
Conversation
aae011b to
a7046db
Compare
a7046db to
3a37b07
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
|
@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. |
|
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. |
72696ed to
0b69c47
Compare
|
Just for the records, a similar change was done in the gaia module: #2967 |
bsipocz
left a comment
There was a problem hiding this comment.
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.
|
Yes, I'm happy with it - but give @ManonMarchand a chance to evaluate my suggested docs rephrasing before we merge. |
59c7ac2 to
e723aa2
Compare
Co-authored-by: keflavich <adam.g.ginsburg@gmail.com>
e723aa2 to
04fcc57
Compare
|
Thank you both! |
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:
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/#tokenLooks like the xsd:token can be
2MASSwithout issues 🙂