Fix inadvertent re-use of base class `allowed_kwargs`

sot / proseco

Probabilistic star evaluation and catalog optimization

https://sot.github.io/proseco

BSD 3-Clause "New" or "Revised" License

1 stars 0 forks source link

Fix inadvertent re-use of base class `allowed_kwargs` #332

Closed taldcroft closed 4 years ago

taldcroft commented 4 years ago

Description

This fixes #330, primarily by 82b3910 which allows setting something that is a class property even if it is not in the allowed_kwargs set.

However, along the way I discovered a more serious problem which was that the class inheritance from ACACatalogTable was re-using (for FidTable and ACATable) the base class allowed_kwargs. This was the source of the ordering issue in #330 because FidTable was trying to set t_ccd but relying on t_ccd showing up in allowed_kwargs because of importing catalog (which defined ACACatalogTable and added t_ccd to allowed_kwargs). So I fixed this problem by making sure each class has a private copy of allowed_kwargs.

Testing

[x] Passes unit tests on MacOS
[x] Functional testing : TO DO add a new unit test.

Fixes #330

jeanconn commented 4 years ago

Thanks @taldcroft . This helps. My brain is still trying to wrap around the bootstrapping it was doing before. I see you've added a test; does that wrap up the action items?

taldcroft commented 4 years ago

The FidTable class was getting the allowed_kwargs attribute as a reference from the base ACACatalogTable. When a MetaAttribute or AliasAttribute attribute is defined, it puts that name into allowed_kwargs. Now FidTable has a t_ccd property, but in set_attrs_from_kwargs it was only checking allowed_kwargs to see if a kwarg t_ccd could be passed into __init__. So when you only import fid.py there was no t_ccd in allowed_kwargs. BUT, if you imported acq.py first, the AcqTable has a t_ccd AliasAttribute and that put t_ccd into the base allowed_kwargs, at which point the t_ccd property setter could be called.

Yup, it is a bit twisted.