latest version can't handle numpy.float64 #39
Comments
|
Thank you for your very useful bug report! The type checking was indeed added in version 0.5.2 (in #37). The error message was very confusing, so I have just pushed a commit that prints a better error message. Old error message (on your code): New error message (on your code): This explains the problem here better: You've passed in a numpy.float64, but when a PoseStamped message is deserialized (such as when you receive it from another ROS node), it will never contain a numpy.float64, but always a float. In that sense, numpy.float64 is the "wrong" type for that field. However, you can put any numeric python type (including numpy types) into any numeric ROS message field, and it will serialize just fine (e.g., when you publish it). You might just lose some precision (e.g. when converting from float to int). If we want to allow that again, we could do it like in the diff below. I'm a bit undecided if that's really what we should be doing. On the one hand, this test allows any messages that won't produce a serialization error. On the other hand, it feels wrong to allow a np.float64 for a byte field. @alecarnevale : Since you implemented the type check in the first place, what is your use case for it? Any opinions? diff --git i/src/rospy_message_converter/message_converter.py w/src/rospy_message_converter/message_converter.py
index 5a6b787..7ac991b 100644
--- i/src/rospy_message_converter/message_converter.py
+++ w/src/rospy_message_converter/message_converter.py
@@ -36,31 +36,36 @@ import rospy
import base64
import sys
import copy
+import numpy as np
+
python3 = (sys.hexversion > 0x03000000)
if python3:
python_string_types = [str]
- python_long_int_types = [int]
+ python_numeric_types = [int, float]
else:
python_string_types = [str, unicode]
- python_long_int_types = [int, long]
+ python_numeric_types = [int, long, float]
+
python_list_types = [list, tuple]
+numpy_numeric_types = [np.int8, np.int16, np.int32, np.int64, np.uint8, np.uint16, np.uint32, np.uint64, np.float32, np.float64]
+
ros_to_python_type_map = {
'bool' : [bool],
- 'byte' : [int],
- 'char' : [int],
- 'float32' : [int, float],
- 'float64' : [int, float],
- 'int16' : [int],
- 'int32' : [int],
- 'int64' : [int],
- 'int8' : [int],
+ 'byte' : python_numeric_types + numpy_numeric_types,
+ 'char' : python_numeric_types + numpy_numeric_types,
+ 'float32' : python_numeric_types + numpy_numeric_types,
+ 'float64' : python_numeric_types + numpy_numeric_types,
+ 'int16' : python_numeric_types + numpy_numeric_types,
+ 'int32' : python_numeric_types + numpy_numeric_types,
+ 'int64' : python_numeric_types + numpy_numeric_types,
+ 'int8' : python_numeric_types + numpy_numeric_types,
'string' : python_string_types,
- 'uint16' : [int],
- 'uint32' : [int],
- 'uint64' : python_long_int_types,
- 'uint8' : [int]
+ 'uint16' : python_numeric_types + numpy_numeric_types,
+ 'uint32' : python_numeric_types + numpy_numeric_types,
+ 'uint64' : python_numeric_types + numpy_numeric_types,
+ 'uint8' : python_numeric_types + numpy_numeric_types
} |
|
The issue is even a bit more nuanced than that. Serialization will only work if the field value is within the interval representable by the datatype. E.g., filling a I've thrown together a quick test script to show this: #!/usr/bin/env python
import sys
import numpy as np
from std_msgs.msg import Byte, Char, Float32, Float64, Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64
MESSAGE_TYPES = [Byte, Char, Float32, Float64, Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64]
NUMPY_NUMERIC_TYPES = [np.int8, np.int16, np.int32, np.int64, np.uint8, np.uint16, np.uint32, np.uint64, np.float32, np.float64]
PYTHON2_NUMERIC_TYPES = [int, long, float]
# "long" doesn't have any max or min value
MAX_VALUES = [sys.maxint, 10 * sys.maxint, sys.float_info.max] \
+ [np.iinfo(t).max for t in [np.int8, np.int16, np.int32, np.int64, np.uint8, np.uint16, np.uint32, np.uint64]] \
+ [np.finfo(t).max for t in [np.float32, np.float64]]
MIN_VALUES = [-sys.maxint, 10 * -sys.maxint, sys.float_info.min] \
+ [np.iinfo(t).min for t in [np.int8, np.int16, np.int32, np.int64, np.uint8, np.uint16, np.uint32, np.uint64]] \
+ [np.finfo(t).min for t in [np.float32, np.float64]]
def serialize_deserialize(message):
"""
Serialize and then deserialize a message. This simulates sending a message
between ROS nodes and makes sure that the ROS messages being tested are
actually serializable, and are in the same format as they would be received
over the network. In rospy, it is possible to assign an illegal data type
to a message field (for example, `message = String(data=42)`), but trying
to publish this message will throw `SerializationError: field data must be
of type str`. This method will expose such bugs.
"""
from io import BytesIO
buff = BytesIO()
message.serialize(buff)
result = message.__class__() # create new instance of same class as message
result.deserialize(buff.getvalue())
return result
for ros_type in MESSAGE_TYPES:
valid_types = []
for numeric_type, max_value, min_value in zip(PYTHON2_NUMERIC_TYPES + NUMPY_NUMERIC_TYPES, MAX_VALUES, MIN_VALUES):
try:
success = True
for value in [max_value, min_value]:
msg = ros_type(data=numeric_type(value))
msg2 = serialize_deserialize(msg)
if msg2.data != value:
success = False
break
if success:
valid_types.append(numeric_type)
except:
pass
print '{}: ['.format(ros_type.__name__), ', '.join(['{}.{}'.format(t.__module__, t.__name__) for t in valid_types]), ']'The (lightly reformatted) output is: You can see that the python builtins (e.g., int, float) don't even show up for most types. That's because it's not always safe to assign any int to a ROS "int8" field, for example. For this reason, I'll probably merge the diff from my previous comment. @alecarnevale , any objections? |
|
If I interpret this correctly, the fix would only work for numpy's types. What was the reasoning to add this check in the first place? With the way python's typing system works, it feels the most fitting to me, if we could just kinda assign anything to everything. Even a np.float64 to a bool (since True and False are defined as 1 and 0 in python) or the other way around. If this gives a serialization error, if the value is out of range its fine, to me. |
I don't know what @alecarnevale 's use case was (he's the one that submitted the PR with the check). However, I believe it's a valuable contribution because it avoids a lot of subtle bugs. I've thought a lot about this and I'm not sure what the most "pythonic" thing to do would be. On the one hand, you're right that python allows conversion from almost anything to anything. On the other hand, numpy also forces you to use stuff like I've added a more restrictive version of my fix above as PR #41. Unless there are any objections, I'm going to merge it tomorrow. Regarding your concerns: Would it be okay for you to add a boolean parameter |
Hi!
This is a very useful package, but I'm having trouble, since I've updated to:
It seems like it can't handle numpy types anymore. Here is an example:
Running it, gives me this error:
That is odd, cause this 1.0 is of type float64.
Can you please look into it?
It does work with version
The text was updated successfully, but these errors were encountered: