od: Extend physical conversion to integer factors, fix typing#673
Open
acolomb wants to merge 7 commits into
Open
od: Extend physical conversion to integer factors, fix typing#673acolomb wants to merge 7 commits into
acolomb wants to merge 7 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! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)