Repository navigation
Parameter() raises TypeError with its documented default allowable_keys=None #110
Description
Found while reviewing #74. Pre-existing on main (b764057); #74 does not change the behavior.
Problem
Parameter.__init__() has allowable_keys: set[str] | None = None, and the docstring lists allowable_keys : set[str] | None, default=None. But the first check rejects anything that is not a set, so the default (or an explicit None) always raises TypeError: `allowable_keys` should be a set of str. The docstring does not say what None should mean. __setitem__() also does key not in self.allowable_keys, which would need a set. The only callers in the package, HH_Param and MKZ_Param, always pass a set.
PySeismoSoil/PySeismoSoil/class_parameters.py
Lines 53 to 54 in b764057
| allowable_keys : set[str] | None, default=None | |
| The allowable parameter names of the constitutive model. |
PySeismoSoil/PySeismoSoil/class_parameters.py
Lines 64 to 65 in b764057
| allowable_keys : set[str] | None | |
| Same as the input parameter. |
| allowable_keys: set[str] | None = None, |
PySeismoSoil/PySeismoSoil/class_parameters.py
Lines 91 to 94 in b764057
| if not isinstance(allowable_keys, set) or any( | |
| not isinstance(_, str) for _ in allowable_keys | |
| ): | |
| raise TypeError('`allowable_keys` should be a set of str.') |
PySeismoSoil/PySeismoSoil/class_parameters.py
Lines 109 to 111 in b764057
| def __setitem__(self, key: str, item: float) -> None: | |
| if key not in self.allowable_keys: | |
| raise KeyError(f"The model does not have a '{key}' parameter.") |
from PySeismoSoil.class_parameters import Parameter
for kwargs in [{}, {'allowable_keys': None}]:
try:
Parameter({'a': 1.0}, **kwargs)
print('OK')
except TypeError as e:
print(f'{type(e).__name__}: {e}')
# TypeError: `allowable_keys` should be a set of str.
# TypeError: `allowable_keys` should be a set of str.
# (expected: the documented default works)
param = Parameter({'a': 1.0}, allowable_keys={'a'})
print(dict(param))
# {'a': 1.0}Fix
Choose one of these:
- Make
allowable_keysa required keyword argument: change the signature toallowable_keys: set[str]with no default, and change the docstring (parameter and attribute) toallowable_keys : set[str]. This matches whatHH_ParamandMKZ_Paramalready do. - Or give
Nonea meaning, for example "any key is allowed": skip the key checks in__init__()and__setitem__()whenself.allowable_keys is None, and say so in the docstring.
Add a test for the chosen behavior: either that Parameter({'a': 1.0}) raises TypeError because the argument is missing, or that it works and accepts new keys.
Activity