DALResults.fieldname_with_ucd way too permissive
- Dominant language
- Python
- Stars
- 88
- Forks
- 64
- Avg merge
- 38m
- Merged PRs (30d)
- 1
Description
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 ce037f28, 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...
Contributor guide
No contributing guide indexed for this repository
Research direction
Start at the fieldname_with_ucd entry point in pyvo.dal and reproduce the example against the PPMXL SCS service. Verify that matching uses the complete UCD rather than a shared atom, then add or update coverage for the reported result and confirm that the exact UCD returns the intended field or None.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- data
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100