`sbpy.data.DataClass._translate_columns` bug?
jianyangli opened this issue · 5 comments
High-level problem description
Looks like sbpy.data.DataClass._translate_columns doesn't actually report KeyError if a columns is not present. This is different from what's stated in the docstring of this method.
What did you do?
import astropy.units as u
from sbpy.data import DataClass
data = DataClass.from_dict({'rh': 1 * u.au, 'delta': 1 * u.au})
assert 'r' in data
assert 'phase' not in data
What did you expect?
The assertions pass
What did really happen?
The second assertion didn't pass. I think this is a bug in sbpy.data.DataClass._translate_columns method because in line 676, it only checks whether the target_columns are in Conf.fieldnames as a whole but didn't check whether its actually in the object.
Provide information on your environment:
operating system and version: [Linux (which distribution?), MacOS, Win]
sbpy version: 0.3.1
astropy version:
numpy version:
@mkelley I'm not sure the original intention of this method, though. Based on the description in the docstring, this would be a bug of this method. But if the original intention was just to check whether the key is in Conf.fieldnames and not to check whether the key was contained in the object, then this is not a bug of this method. And in this case, we should fix the problem in __contains__ method. Please advise. Thanks.
Good catch. I agree it is a bug based on the description. Also, Orbit.from_oo relies on a KeyError from _translate_columns if the field isn't present in the data. In addition, Orbit.from_oo and Orbit.oo_propagate rely on a KeyError if the field isn't present in the data.
It may have been missed because the last line in __getitem__ throws a KeyError when the requested field isn't present, and _convert_columns wouldn't normally be used with a field name that isn't present in the table. Similarly, Orbit.oo_transform will raise KeyError in a different part of the code (but this would happen if we fixed the bug, too).
test_dataclass.py::test_translate_columns_and_contains() does not test this scenario, but should!
OK, I'll try to fix it in _translate_columns. Not familiar with oo-related methods, though. In fact I'm having trouble installing openorb in my system. But I'll take a look and see if there is any related problems to fix. I don't see Orbit.from_oo, but just a static method Orbit._from_oo. Was that what you referred to?
Hopefully one of the tox environments can still install pyoorb for you for testing (e.g., tox -e py38-test-alldeps). I've made a pip installable version at https://github.com/mkelley/pyoorb-experiment (it requires gfortran). (and yes it was _from_oo)
OK, I got pyoorb installed using your pip installable version, but had to add an ARCHFLAGS="-arch x86_64" environment setting in front of the pip command, otherwise it would fail with an error message:
clang: error: the clang compiler does not support '-march=native'
Then I had to update my numpy to the latest 1.23.1 in order to import pyoorb correctly. Totally no idea what was going on, but glad everything seems to work.
Looks like Orbit class works fine. I'll open a PR so you can check it.