Use integer division in encode_phys to prevent rounding errors with int64

[yves-chevallier]

While testing int64 values with SDOs, I observed an unexpected behavior:

>>> value = 0x55554444AAAABBBB
>>> d.node.sdo[16384][4].phys = value
>>> hex(d.node.sdo[16384][4].phys)
'0x55554444aaaabc00'

Upon investigation, it appears that the issue originates from the encode_phys function:

>>> value = 0x55554444AAAABBBB
>>> sdo.od.factor
1
>>> hex(sdo.od.encode_phys(value))
'0x55554444aaaabc00'

The problem lies in the fact that the division is performed using floating-point arithmetic rather than integer arithmetic, leading to rounding that causes a loss of up to 10 bits of precision.

def encode_phys(self, value: Union[int, bool, float, str, bytes]) -> int:
    if self.data_type in INTEGER_TYPES:
        value /= self.factor
        value = int(round(value))
    return value

To address this, we should detect whether the input value is an integer and, if so, perform integer division (//) instead of floating-point division. Additionally, it may be prudent to emit a warning when factor is not an integer, as this could lead to precision loss.

[acolomb]

I'm not convinved this is the right approach. Truncating may be worse than rounding in some existing use cases, so we should avoid breaking them / changing to different values unexpectedly. OTOH, you can always do the calculation yourself and assign to .raw instead. Which might be better for just the few cases where large integers cause inaccuracy.

[acolomb]

round() already returns an integer if ndigits is omitted.

[yves-chevallier]

Round was the original code. My contribution is only the // division, which is my use case is the right thing to do. What do you suggest?

[acolomb]

If you touch / edit this code and such a useless call is found, please remove it as part of your change.

[sveinse]

The problem lies in the fact that the division is performed using floating-point arithmetic rather than integer arithmetic, leading to rounding that causes a loss of up to 10 bits of precision.

I think TS has a point here. It should be possible to reliably pass int64 through the encode function.

To address this, we should detect whether the input value is an integer and, if so, perform integer division (//) instead of floating-point division.

This could be a good solution since 64-bit double can only hold up to 52 bits (+ sign bit) in the mantissa, so any values higher than that will get truncated in the float division.

Additionally, it may be prudent to emit a warning when factor is not an integer, as this could lead to precision loss.

Is it a use case to have non-integer factors? When do we get into this use case? Does the standard permit float scale factors?

Another alternative would be to skip the division altogether if the factor is 1. In most cases factor is not used at all, and I'd like to suggest that int64 with scale factor other than 1 is somewhat uncommon use case.

[acolomb]

I'm okay with bypassing the division altogether if the factor is one, as in int(1). That is also the default value and if no conversion is needed, I'd recommend to just use .raw instead of .phys.

Is it a use case to have non-integer factors? When do we get into this use case? Does the standard permit float scale factors?

For example we want a quantity expressed as a percentage 0.0 ... 100.0 , but the bus representation is 0 ... 65535. I'd put in .factor = 100 / 65535. The CiA 306 standard does not mention the unit and factor attributes at all AFAIK, those are extensions specific to this library (and adopted by some other implementations).

I see it as a breaking change if suddenly integer division is applied for a perfectly valid use-case that previously returned a properly rounded value. An integer factor can only upscale, such as mapping a quantity in meters into a common range with other variables that work with millimeters: .factor = 1000. When I want to send back such a value, which might have higher precision from an internal calculation, this PR changes what is sent: 5555 mm // 1000 mm/m = 5 m, where previously it would have resulted in round(5555 mm / 1000 mm/m) = round(5.555 m) = 6 m.

That's a subtle difference but could change some systems' behavior in non-trivial and hard to detect ways. Pretty high risk for fixing an edge case which should better be avoided altogether by doing the calculation as needed outside the library, then using the .raw member. At least I'd want some unit tests showing the current results and making sure they don't change inadvertently for common cases. That documents clearly what to expect from the conversion in edge cases and could also serve to show the problem at hand.

By the way, if I read this code (current or from the PR) correctly, then we don't even apply the conversion if the OD variable is already a float?

[sveinse]

I'm okay with bypassing the division altogether if the factor is one, as in int(1). That is also the default value and if no conversion is needed, I'd recommend to just use .raw instead of .phys.

I still would expect .phys with scale factor 1 to not have side-effects.

I see it as a breaking change if suddenly integer division is applied for a perfectly valid use-case that previously returned a properly rounded value. An integer factor can only upscale, such as mapping a quantity in meters into a common range with other variables that work with millimeters: .factor = 1000. When I want to send back such a value, which might have higher precision from an internal calculation, this PR changes what is sent: 5555 mm // 1000 mm/m = 5 m, where previously it would have resulted in round(5555 mm / 1000 mm/m) = round(5.555 m) = 6 m.

A simple fix is to use integer division only if both operands are int and the dividend is not fully representable in float.

[acolomb]

A simple fix is to use integer division only if both operands are int and the dividend is not fully representable in float.

A simple fix for what? For the corner case described in the OP?

My suggested path is this: if self.factor is int(1) we skip the division altogether. This will also fix the initial problem, as then no division is performed at all. When a factor different from int(1) has been configured, it's really up to the library user to take care of avoiding overflow, as there will always be a factor-dependent limit to what input values can be handled without loss of precision. I don't see us doing multiple calculations and comparing the result or whatnot just to avoid that. If you're working close to the limits of the representable numeric range, then you need to take extra care in an application-specific way and should probably better use the .raw attribute from the start.

What we can do is to avoid breaking the round-trip behavior with default settings, as demonstrated in the OP. And this is solved with a simple self.factor is int(1) check.

Any other changes would need to be accompanied by unit tests showing where the old behavior is wrong, how any changed behavior is better, and how some common cases (not close to the extreme limits) do not lead to different results. Because the latter would be strictly worse.

[bizfsc]

I agree with @acolomb's latest suggestion. The simplest and safest approach is:

def encode_phys(self, value):
    if self.data_type in INTEGER_TYPES:
        if self.factor != 1:
            value = int(round(value / self.factor))
    return value

This solves the original problem (int64 precision loss with factor=1) without changing behavior for any other factor. Integer division (//) would be a subtle breaking change for existing users who rely on the rounding behavior with integer factors like factor=1000.

For users working with large integers and non-unity factors, using .raw directly is the right approach, as @acolomb pointed out. The library should not try to be too clever here — there will always be a factor-dependent precision limit with float division, and that's inherent to the math, not a bug.

Also agree that unit tests should accompany this change to document the expected behavior for:

• factor=1 with large int64 values (the original bug)
• factor=1000 with values that should round (existing behavior preserved)
• factor as a float (existing behavior preserved)
• The int(round(...)) → round(...) cleanup that @acolomb suggested (since round() already returns int when ndigits is omitted)

If you want, then I can create a new PR.

[acolomb]

@bizfsc Great if you want to pick this up, as there has been no response in a long time. Note that your snippet uses != 1 though instead of the proposed is not 1. Setting the factor to float 1.0 makes sense to always get a float as physical representation. But this inverse calculation always converts back to int anyway, so I guess != 1 is alright. Dunno which one is more efficient in the end.

[bizfsc]

According to different sources in python "is, is not" is used to compare identity (object address) and "==, !=" to compare values, which is the case here. In the end there shouldn't be a measurable difference because python caches all integers up to 256 or something like that.

I created PR #648