Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DALResults.fieldname_with_ucd way too permissive #566

Open
msdemlei opened this issue Jun 27, 2024 · 0 comments
Open

DALResults.fieldname_with_ucd way too permissive #566

msdemlei opened this issue Jun 27, 2024 · 0 comments

Comments

@msdemlei
Copy link
Contributor

Consider the following code:

import pyvo

svc = pyvo.dal.SCSService("https://dc.g-vo.org/ppmxl/q/cone/scs.xml")
resultset = svc.search((10, 10), radius=0.01)
print(resultset.fieldname_with_ucd("pos.pm;pos.eq.ra"))

This currently prints e_raepRA, which is highly bizarre (it's the error in RA, nb not the error in pmra or something like that).

Looking at the implementation, this is not surprising: What the method picks is the first column with a UCD that shares an atom with the UCD passed in. Git blame tells me that this implementation was introduced in 2018 in commit ce037f2, with the (for this purpose) unhelpful commit message:
"use pytest and mock rather than legacy unittest and background server".

The previous implemenation,

    try:
        iterchain = (
            self.getdesc(fieldname) for fieldname in self.fieldnames)
        iterchain = (field for field in iterchain if field.ucd == ucd)
        return next(iterchain).name
    except StopIteration:
        return None

seemed sensible (modulo the somewhat complicated formulation), so I wonder why Stefan would have made things so permissive, and that while fiddling with the test system.

And I have a hard time believing I'm the first to bite this. Anyway: does anyone see a strong reason not to go back to what this did before, which simplifies to:

for field in self.fielddescs:
  if field.ucd == ucd:
    return field.name

In case this is really too stringent in that we may want to discard a meta.main (or something like that) occasionally, we can perhaps add extra modes. But I'm really sure we should not be too clever here by default; that's a recipe for WTF moments...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant