Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MultiArrayDimension.stride is bad #8

Closed
eric-wieser opened this issue Feb 27, 2016 · 10 comments
Closed

MultiArrayDimension.stride is bad #8

eric-wieser opened this issue Feb 27, 2016 · 10 comments

Comments

@eric-wieser
Copy link

In MultiArrayLayout.msg, the definition given is:

multiarray(i,j,k) = data[data_offset + dim_stride[1]*i + dim_stride[2]*j + k]
#
# A standard, 3-channel 640x480 image with interleaved color channels
# would be specified as:

dim[0].label  = "height"
dim[0].size   = 480
dim[0].stride = 3*640*480 = 921600  (note dim[0] stride is just size of image)
dim[1].label  = "width"
dim[1].size   = 640
dim[1].stride = 3*640 = 1920
dim[2].label  = "channel"
dim[2].size   = 3
dim[2].stride = 3

# multiarray(i,j,k) refers to the ith row, jth column, and kth channel.

This is a fundamentally asymmetric definition:

  • dim[0].stride has no meaning
  • There is no way to specify the stride of the last index

I would like to contest that this defeats one of the main purposed of striding, which is to allow memory order to be exchanged. Instead, the indexing rule should be:

data[data_offset + dim_stride[0]*i + dim_stride[1]*j + dim_stride[2]*k]

And in our example, the dimensions would be

dim[0].label  = "height"
dim[0].size   = 480
dim[0].stride = 3*640 = 1920 # how far do I need to step to get to the next row
dim[1].label  = "width"
dim[1].size   = 640
dim[1].stride = 3 # how far do I need to step to get to the next pixel
dim[2].label  = "channel"
dim[2].size   = 3
dim[2].stride = 1 # how far do I need to step to get to the next channel

Note this allows us to serialize our array in a different order as [red data] [green data] [blue data], and then label the axes differently:

dim[0].label  = "height"
dim[0].size   = 480
dim[0].stride = 640 # moving to the next row means stepping over the width
dim[1].label  = "width"
dim[1].size   = 640
dim[1].stride = 1  # pixels in a row are adjacent
dim[2].label  = "channel"
dim[2].size   = 640*480  # moving to the next channel means stepping over a whole image
dim[2].stride = 1

This makes the definition of stride match up with the numpy definition of stride.

@tfoote
Copy link
Member

tfoote commented Mar 2, 2016

This looks like a reasonable suggestion. Unfortunately I think that the design meeting notes for this message were on the old wiki and consequently are going to be hard to come by.

As a general note these messages were some of the first complex messages developed in ROS and were originally designed to be the underlying datatypes for Images and PointClouds. However they were determined that they could not capture all encodings for images. And for point clouds we mostly wanted to use external tools (ala pcl) for processing the point clouds instead of being restricted to the built in indexing.

These super generic datatypes also lost their semantic meaning because they are so generic making introspection and debugging much harder. (If you recieve a Int8MultiArray on the wire do you pass it to an image viewer or a point cloud viewer?) As such they've basically been unused, but left for quick prototyping so as not to break anyone already using them. Also they required the complex indexing which clearly (#9) is vulnerable to getting wrong.

This level of change of a core datatype is something that would break everything using it in it's current form. To not break any current user a new datatype could be added. But from our experience with the lack semantic meaning, I would highly recommend making the a very similar datatype but give it a non-generic name that means something, and consider adding any extra metadata fields which are appropriate for that semantic meaning.

@eric-wieser
Copy link
Author

Is there scope for type parameters in ROS messages in future? For example, MultiArray<int>, PointCloud2<float, float, int> or Stamped<Vector>? It seems like there's a lot of message duplication, and therefore loss of semantic meaning, for working around this absense,

@eric-wieser
Copy link
Author

These super generic datatypes also lost their semantic meaning because they are so generic making introspection and debugging much harder

I'd argue their main purpose should be something like:

MyMessageType.msg

Header header;
SomeExtraInfo info
IntMultiArray theUnderlyingData

Because that gives an uniform implementation under the hood for different message types

@tfoote
Copy link
Member

tfoote commented Mar 2, 2016

Templating is not possible in all languages.

To start off with, if you're making a custom message you might as well collapse one layer. Otherwise you have to address data with

my_msg.theUnderlyingData.data[index]
or with a slightly shorter naming my_msg.data.data[index] which starts to look verbose.

vs below would just be

my_msg.data[index]

from a definition like:

Header head
SomeExtraInfo info
MultiArrayLayout  layout
int8[] data

And although n-dimentional arrays are a very elegant in the pure CS sense. The number of use cases needing to support n-dimentional arrays of data are small, and those that can share code are even rarer. Thus supporting the common use cases with customized simpler datatypes provides a lot more value. For example an Image and a MultiEchoLaserScan look very similar data blocks (a 2 dimentional array of data), but semantically they cannot be treated the same at all. The type provides semantic meaning and there's no mistaking a multi echo laser scan for an image and trying to view it with the same tools, nor run it through the wrong type of filters.

@eric-wieser
Copy link
Author

Templating is not possible in all languages.

What's the intersection of that set with ros supported languages? Python would work as MultiArray[int], which is the direction the new type annotations went in. Even in something like C, you could have

DECLARE_MSG_TYPE_WITH_PARAMS(MultiArray,int); // produces a definition of MultiArray__int
void mySubscriber(MultiArray__int msg) { ... }

To start off with, if you're making a custom message you might as well collapse one layer

But this could happen at the language runtime layer - just as Time is extracted into a special type in rospy, the same could be done to multidimensional arrays

Image and a MultiEchoLaserScan look very similar data blocks (a 2 dimentional array of data), but semantically they cannot be treated the same at all.

Actually, i think that's a bad example. A MultiEchoLaserScan is basically a spherical depth map, right? And in ROS, depth maps are often transmitted as images. Sure, they're not exact matches to each other, but in some cases you might not care.

In those cases, having to convert one custom array format into another is a huge waste of programmer and processing time. If memory layout could be standardized, then, this burden is lifted from both - if you decide this sort of data conversion is a good idea, the code would just be image_msg.data = echo_msg.data

@tfoote
Copy link
Member

tfoote commented Mar 7, 2016

One quick comment that the MultiEchoLaserScan is not a spherical depth map at all, the readings are on the same ray at different depths.

We discussed this in the ROS team meeting and the disruptive nature of changing the indexing means that we cannot change it in the existing message. If there's an existing use case, we suggest creating a new message definition which covers the use case and provides a semantically meaningful name and documentation.

@codebot is going to add some documentation also to explain how the messages in std_msgs are for examples and quick prototyping but are not recommended for regular reuse in due to their lack of semantic meaning.

@codebot
Copy link

codebot commented Mar 8, 2016

I added a bit of text to the std_msgs wiki page to summarize this.

@codebot
Copy link

codebot commented Mar 8, 2016

Also updated the one-line description of this package in the github repo.

@eric-wieser
Copy link
Author

It's a bit weird how there's one message in this package (Header) that everything should use, and yet the rest are best for prototyping only

@codebot
Copy link

codebot commented Mar 8, 2016

Yeah, I agree completely, it's weird. Unfortunately, I don't have a good idea for how to separate the package into "useful for prototyping" and "useful for everything" since this package is ~8 years old now, and basically everyone using ROS is using this package either directly or indirectly. I'm almost too afraid to touch this package at all, to be honest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants