Allow NaN values to pass floating point bounds check.

[oysstu]

This pull request resolves the issue #162 by explicitly adding NaN values to the floating point bounds check.

Signed-off-by: Oystein Sture os@skarvtech.com

Fixes #169
Fixes #162

[oysstu]

Math should only be imported for messages with floating point fields in order for it to pass the linter

[oysstu]

Updated the pull request. Inverted the bounds check instead of calling math.isnan. This works since NaN always evaluates to false in comparisons.

[sloretz]

Thank you for the ticket and PR!

Would you be willing to update it to do two more things?

• Modify the comparison logic to allow positive and negative infinity (-float('inf') and float('inf'))
• Add tests for NaN, -inf, and inf for float32 and float64 types in test_basic_types and test_arrays

Playground script for myself when I re-review

import math


bound = 3.402823e+38


def old_float_check(value):
    return value >= -bound and value <= bound


def float_check(value):
    return not (value < -bound or value > bound)


def float_array_check(value):
    return not any(val < -bound or val > bound for val in value)


def alt_check(value):
    return (not (value < -bound or value > bound)) or math.isinf(value)


def alt_array_check(array_value):
    return all((not (value < -bound or value > bound)) or math.isinf(value) for value in array_value)


test_values = [
    float('nan'),
    float('inf'),
    -float('inf'),
    0,
    float(0),
    -float(0),
    3.14,
    -3.14,
]


print("New check")
for v in test_values:
    print(f'{float_check(v)} - {repr(v)}')
print(f'{repr(test_values)} - {float_array_check(test_values)}')


print ("Old check")
for v in test_values:
    print(f'{old_float_check(v)} - {repr(v)}')


print ("Alternative check")
for v in test_values:
    print(f'{alt_check(v)} - {repr(v)}')
print(f'{repr(test_values)} - {alt_array_check(test_values)}')

[oysstu]

In that case, the bounds check for float64 can be removed, as all numbers pass through it, unless I'm misunderstanding something about the representation of the values. Assigning a larger number than 1.7976931348623157e+308 to a regular python float or numpy.float64 automatically turns it into inf.

The bounds check may still make sense for float32, since there is no python builtin type, and may exceed the C/C++ float type. Question is if it should be converted to -inf/inf instead of throwing a bounds error, since this is the behavior for float64.

[connoranderson]

Question is if it should be converted to -inf/inf instead of throwing a bounds error, since this is the behavior for float64.

Just my opinion here, I think silent overflow to inf makes sense for float32, if we do not have type-checking to validate that the user is actually providing a np.float32.

[oysstu]

Question is if it should be converted to -inf/inf instead of throwing a bounds error, since this is the behavior for float64.

Just my opinion here, I think silent overflow to inf makes sense for float32, if we do not have type-checking to validate that the user is actually providing a np.float32.

I agree. And that seems to be the behavior in IEEE 754 as far as I can tell. Honestly, I'm in favor of removing the entire bounds check for floating point numbers. I don't see what problem it solves.

[connoranderson]

Yes it looks like IEEE 754 specifies this behavior

Maybe @clalancette could weigh in on why the original PR was merged and then we can figure out whether the bounds check should be kept?

[sloretz]

The bounds check may still make sense for float32, since there is no python builtin type, and may exceed the C/C++ float type. Question is if it should be converted to -inf/inf instead of throwing a bounds error, since this is the behavior for float64.

Honestly, I'm in favor of removing the entire bounds check for floating point numbers. I don't see what problem it solves.

The idea behind the checks using @property on the python classes was to make it easier to debug fields being set with invalid values. It raises an error at the point in code that a message was given an invalid value rather than getting an error when the message can't be serialized. The downside is these checks can be pretty expensive when setting a lot of fields. The checks can be skipped by passing the -O option to cPython. Checking the bounds of float32 here seems like it matches that intent.

Letting the conversion to inf happen means a message serialized and then deserialized might not be equal to itself, which feels non-intuitive to me.

from rclpy.serialization import serialize_message, deserialize_message

from test_msgs.msg import BasicTypes


def float32_value(self):
   return self._float32_value


def float32_value_setter(self, value):
   self._float32_value = value


BasicTypes.float32_value = property(float32_value, float32_value_setter)


if __name__ == '__main__':

    b = BasicTypes()
    b.float32_value = -3.402823e+40
    b_prime = deserialize_message(serialize_message(b), BasicTypes)

    print('b == b_prime?', b == b_prime)  # prints False because b_prime.float32_value is inf

As for float64, I agree the bounds check isn't needed as long as sys.float_info.max and sys.float_info.min are the same as the bounds we'd be checking. If we get rid of the bounds check for float64 I'd still recommend a check that sys.float_info matches what we expect when the class is constructed.

[oysstu]

Letting the conversion to inf happen means a message serialized and then deserialized might not be equal to itself, which feels non-intuitive to me.

I agree that this is undesirable. If the corresponding numpy type or python.ctypes type had been used in python, this would not be the case.

As for float64, I agree the bounds check isn't needed as long as sys.float_info.max and sys.float_info.min are the same as the bounds we'd be checking. If we get rid of the bounds check for float64 I'd still recommend a check that sys.float_info matches what we expect when the class is constructed.

That would mean that python is running on hardware that is not IEEE 754 compliant, or compiled with flags that disable conformance mode, but yes it can happen.

What if the floating point bounds checks are kept, but variables explicitly set to Inf/NaN are allowed through? How does that sound?

[sloretz]

What if the floating point bounds checks are kept, but variables explicitly set to Inf/NaN are allowed through? How does that sound?

That sounds great to me

[oysstu]

I think the tests are failing because the assigned value gets rounded to inf and passes the bounds check, but I'll need to have a closer look at why it's happening. I won't have time to look at this again for a few days

Changes since review

[oysstu]

I've updated the pull request, and it seems to pass the checks now. The float64 bounds check test is only executed if sys.float_info.max exceeds 1.7976931348623157e+308. Otherwise, the python float is converted silently to inf, which passes the bounds check without triggering the assertion error.

The out or range tests are performed in test_arrays(), test_bounded_sequences(), and test_unbounded_sequences(). I've added the new tests for all of them. Perhaps these can be cleaned up in a separate PR.

[sloretz]

LGTM with green CI!

[sloretz]

CI (repos file build: --packages-above-and-dependencies rosidl_generator_py test: --packages-above rosidl_generator_py)

• Linux
• Linux-aarch64
• Windows

[sloretz]

CI LGTM!

Thank you for the PR and for iterating!

[mmatthebi]

Hi, is there any plan when and if this PR will be merged into current Humble release?

[sloretz]

Hi, is there any plan when and if this PR will be merged into current Humble release?

I'm cautious about backporting a behavior change to a low level package like this. If it is backported, then #182 must be backported with it.

[mmatthebi]

Hi, is there any plan when and if this PR will be merged into current Humble release?

I'm cautious about backporting a behavior change to a low level package like this. If it is backported, then #182 must be backported with it.

thanks for the quick answer. In my eyes, this behaviour in Humble is a regression from Galactic, hence worth porting back into Humble. I actually wonder why this has not impeded more users. Our system relies on setting some message contents to nan to indicate that the value is empty and this is actually also commonly used in built-in messages (like BatteryState). With the present behaviour we cannot move our setup to Humble LTS (which we would rather prefer).

[markcutler]

Hi, is there any plan when and if this PR will be merged into current Humble release?

I'm cautious about backporting a behavior change to a low level package like this. If it is backported, then #182 must be backported with it.

thanks for the quick answer. In my eyes, this behaviour in Humble is a regression from Galactic, hence worth porting back into Humble. I actually wonder why this has not impeded more users. Our system relies on setting some message contents to nan to indicate that the value is empty and this is actually also commonly used in built-in messages (like BatteryState). With the present behaviour we cannot move our setup to Humble LTS (which we would rather prefer).

+1 This is affecting our system as well. We would like to upgrade to Humble but need this PR backported first. Thanks.

[fujitatomoya]

@mergify backport humble

[mergify]

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission>=write

[fujitatomoya]

https://github.com/Mergifyio backport humble

[fujitatomoya]

@Mergifyio backport humble

[mergify]

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission>=write

[mergify]

backport humble

❌ Command disallowed due to command restrictions in the Mergify configuration.

• sender-permission>=write

[fujitatomoya]

@clalancette @sloretz i think this can backport to humble, what do you think? (I do not have permission to manage backport though.)