Add support for CompressedImage, XXXStamped and messages from rosbag #6
Conversation
test/test_from_bag.py
Outdated
| # ros-indigo-geometry-msgs 1.11.9-0trusty-20160627-170848-0700 | ||
| # Pose msg has md5sum e45d45a5a1ce597b249e23fb30fc871f | ||
| # so we use one made up from: | ||
| from different_md5_Pose import Pose as different_md5_Pose |
There was a problem hiding this comment.
@eric-wieser If you are wondering why I added a __init__.py file to the test folder, is so I can import this custom Pose message definition with a different md5sum. (the _md5sum field is a read-only attribute so I didn't find another way of testing with a different md5sum).
There was a problem hiding this comment.
This won't work on python 3 unless you change it to from .different_md5_Pose import Pose.
Instead of bundling that file, could you just generate a test message as part of the test cmake section?
There was a problem hiding this comment.
Done on the from import.
I'm very unfamiliar with cmake... and it's just a test, I can't really work on it right now.
| return points_arr | ||
|
|
||
| # From http://stackoverflow.com/questions/902761/saving-a-numpy-array-as-an-image | ||
| def write_png(buf, width, height): |
There was a problem hiding this comment.
I'd prefer not to build a mini PNG library into this package! How about:
from io import BytesIO
import imageio
def write_png(arr):
b = BytesIO()
imageio.imwrite(b, arr, format='png')
return b.getvalue()There was a problem hiding this comment.
I prefer to not pull another library as dependency, even being a test-only dependency.
It's just a test and it works, sorry to not agree.
There was a problem hiding this comment.
Why is writing a PNG even a necessary part of this test? That seems pretty out of scope for checking the mangling of type ids by rosbag, which is what this PR is about, right?
test/test_from_bag.py
Outdated
| # ros-indigo-geometry-msgs 1.11.9-0trusty-20160627-170848-0700 | ||
| # Pose msg has md5sum e45d45a5a1ce597b249e23fb30fc871f | ||
| # so we use one made up from: | ||
| from different_md5_Pose import Pose as different_md5_Pose |
There was a problem hiding this comment.
This won't work on python 3 unless you change it to from .different_md5_Pose import Pose.
Instead of bundling that file, could you just generate a test message as part of the test cmake section?
|
Please ping me more often on these PRs - I have a very high volume of github mail from http://github.com/numpy/numpy issues, and so these get lost underneath. |
| # looking like '_sensor_msgs__Image' looks like tmpldS_vh._sensor_msgs__Image | ||
| # so we try to check for the _type instead ('sensor_msgs/Image') | ||
| for class_instance, _ in _to_numpy.keys(): | ||
| if msg._type == class_instance._type: |
There was a problem hiding this comment.
Comment here saying that this checks that package/name is the same for both would be nice
There was a problem hiding this comment.
Nevermind, the comment about that above is pretty clear
| @@ -32,6 +32,21 @@ def numpify(msg, *args, **kwargs): | |||
| raise ValueError("Cannot determine the type of an empty Collection") | |||
| conv = _to_numpy.get((msg[0].__class__, True)) | |||
There was a problem hiding this comment.
need to hit this path too.
I suggest moving your checks to a helper _normalize_rosbag_cls function (or a better name), then you can just drop in here and on line 29:
cls = normalize_rosbag_cls(msg[0].__class__)
| else: | ||
| raise ValueError("Unable to convert message of type {}, ".format(msg._type) + | ||
| "the md5sum of message to numpify and the installed message differ: {} != {}".format( | ||
| msg._md5sum, class_instance._md5sum) |
There was a problem hiding this comment.
Concatenating formatted strings like this is unusual -I'd normally go with
raise ValueError(
"line {}, with no + at the end, "
"line {}".format(
1, 2
)
)
| # on messages coming from a rosbag, __class__ instead of | ||
| # looking like '_sensor_msgs__Image' looks like tmpldS_vh._sensor_msgs__Image | ||
| # so we try to check for the _type instead ('sensor_msgs/Image') | ||
| for class_instance, _ in _to_numpy.keys(): |
There was a problem hiding this comment.
class_instance is pretty ambiguous - I'd opt for just cls, which is the pythonic name for something where isinstance(cls, type) is true.
In this PR I expand from #5
It adds CompressedImage and XXXXStamped support like #5 but it also adds support for rosbags following your comments on #3 (comment) (it also fixes #2).
Now, when we encounter a message of the same type but with a different md5sum, it will raise a
ValueErrorexplaining what's going on with the md5sums.This could actually be checked for any message, not only the ones coming from a rosbag, but it's a more rare use case I guess.
I hope it feels less like a hack now.
Btw, if you'd like, I could also be added as a maintainer to the package. It should be version bumped and re-released :) (please)