Skip to content

od: Extend physical conversion to integer factors, fix typing#673

Open
acolomb wants to merge 8 commits into
canopen-python:masterfrom
acolomb:phys-passthrough
Open

od: Extend physical conversion to integer factors, fix typing#673
acolomb wants to merge 8 commits into
canopen-python:masterfrom
acolomb:phys-passthrough

Conversation

@acolomb

@acolomb acolomb commented Jun 4, 2026

Copy link
Copy Markdown
Member

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.

Allow integer factors in type hint for ODVariable.factor. 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.

Thus, try to parse integer factors as integers from EDS files, not forcing
to a float.

Related comment first given here: #651 (comment)

acolomb added 4 commits June 4, 2026 22:48
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

codecov Bot commented Jun 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@acolomb acolomb marked this pull request as ready for review June 4, 2026 21:36
@acolomb acolomb added the typing Concerns typing information to be consumed by library users. label Jun 14, 2026

@bizfsc bizfsc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to zero runtime cost and without assert.
Pylance / mypy happy.

Comment thread test/test_od.py
self.assertIsInstance(encoded, int)

def test_phys_int_factor_decodes_to_int(self):
"""decode_phys with float factor ensures a float result."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""decode_phys with float factor ensures a float result."""
"""decode_phys with int factor ensures a int result."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring nit

Comment thread canopen/objectdictionary/eds.py Outdated
try:
var.factor = float(eds.get(section, "Factor"))
except ValueError:
pass

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Comment thread canopen/objectdictionary/__init__.py Outdated
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))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert isinstance(value, (int, float))

decode_raw ensure correct typing. no assert in production code. see bigger comment for both functions

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    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 value

Used too much brain power to solve this issue for the current temperatures. But here is the dilemma:

  1. don't use assert in production
  2. don't change signature
  3. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

typing Concerns typing information to be consumed by library users.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants