od: Extend physical conversion to integer factors, fix typing#673
od: Extend physical conversion to integer factors, fix typing#673acolomb wants to merge 8 commits into
Conversation
The Variable.phys property docstring mentions "Non integers will be passed as is." That is correct, but not reflected in the type hints for the associated ODVariable.encode_phys() / .decode_phys() functions. Extend the return / argument type to all possible internal data types, instead of plain int.
Try factor conversion on all NUMBER_TYPES, where it was previously limited to INTEGER_TYPES. Only apply rounding in encode_phys() if the variable's data type is actually integer-based.
There is nothing wrong with using integer factors to yield integers when decoding. For encoding, the division may still result in a float, while the rounding behavior is kept unchanged for integer-typed variables.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bizfsc
left a comment
There was a problem hiding this comment.
Changed to zero runtime cost and without assert.
Pylance / mypy happy.
| self.assertIsInstance(encoded, int) | ||
|
|
||
| def test_phys_int_factor_decodes_to_int(self): | ||
| """decode_phys with float factor ensures a float result.""" |
There was a problem hiding this comment.
| """decode_phys with float factor ensures a float result.""" | |
| """decode_phys with int factor ensures a int result.""" |
| try: | ||
| var.factor = float(eds.get(section, "Factor")) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
| pass | |
| logger.warning( | |
| "Could not parse Factor for %s in section [%s], ignoring", | |
| var.name, section, | |
| ) |
Yes, we don't have this in many places, but lets get this started. Ar least in debugging the devs can see now, that the parsing failed.
| self, value: Union[int, bool, float, str, bytes] | ||
| ) -> Union[int, bool, float, str, bytes]: | ||
| if self.data_type in NUMBER_TYPES: | ||
| assert isinstance(value, (int, float)) |
There was a problem hiding this comment.
| assert isinstance(value, (int, float)) |
decode_raw ensure correct typing. no assert in production code. see bigger comment for both functions
There was a problem hiding this comment.
def decode_phys(
self, value: Union[int, bool, float, str, bytes]
) -> Union[int, bool, float, str, bytes]:
if self.data_type in NUMBER_TYPES:
numeric = cast(Union[int, float], value)
value = numeric * self.factor
return value
def encode_phys(
self, value: Union[int, bool, float, str, bytes]
) -> Union[int, bool, float, str, bytes]:
if self.data_type in NUMBER_TYPES:
numeric = cast(Union[int, float], value)
if self.factor != 1:
numeric = numeric / self.factor
if self.data_type in INTEGER_TYPES:
numeric = round(numeric)
value = numeric
return valueUsed too much brain power to solve this issue for the current temperatures. But here is the dilemma:
- don't use assert in production
- don't change signature
- don't ask for permission, ask for forgiveness
=> cast is noop in runtime = zero loss and the linter is happy, because the can not see that only int or float can be passed. This should be the best solution. Also numeric / self.factor will raise natural type error for wrong types
The
Variable.physproperty docstring mentions "Non integers will bepassed as is." That is correct, but not reflected in the type hints
for the associated
ODVariable.encode_phys()/.decode_phys()functions. Extend the return / argument type to all possible internal
data types, instead of plain
int.Try factor conversion on all
NUMBER_TYPES, where it was previouslylimited to
INTEGER_TYPES. Only apply rounding inencode_phys()if thevariable's data type is actually integer-based.
Allow integer factors in type hint for
ODVariable.factor. There isnothing wrong with using integer factors to yield integers
when decoding. For encoding, the division may still result in a
float, while the rounding behavior is kept unchanged for integer-typedvariables.
Thus, try to parse integer factors as integers from EDS files, not forcing
to a
float.Related comment first given here: #651 (comment)