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

Use numpy.array's "offset" param instead of slicing #33

Open
wants to merge 1 commit into
base: indigo-devel
Choose a base branch
from

Conversation

abencz
Copy link

@abencz abencz commented Nov 26, 2014

Passing an offset into numpy.array instead of slicing should prevent an
additional copy of the data being created by the slice operation. This is
important for really big message types like OccupancyGrids.

Documentation of the offset parameter:
http://docs.scipy.org/doc/numpy/reference/generated/numpy.frombuffer.html

@mikepurvis
Copy link
Member

@dirk-thomas, any thoughts on this?

@dirk-thomas
Copy link
Member

It would be great to improve the performance of this by avoiding the extra copy.

The current patch breaks the public API (introducing the offset argument in the middle of other positional arguments). Can you please change that to something non-breaking, e.g. an optional keyword argument?

I haven't checked how well this code path is covered by a unit test. If not could you please also add a unit test to ensure the new feature doesn't break in the future?

@dirk-thomas
Copy link
Member

If anyone is interested enough in this feature to update the PR with the above comments that would be highly appreciated?

@dirk-thomas dirk-thomas added this to the Untargeted milestone Aug 5, 2015
@abencz
Copy link
Author

abencz commented Feb 3, 2016

I wasn't aware that genpy, and especially unpack_numpy was part of the public API of ROS. I'll update the PR.

Passing an offset into numpy.array instead of slicing should prevent an
additional copy of the data being created by the slice operation. This
is important for really big message types like OccupancyGrids.
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

Successfully merging this pull request may close these issues.

None yet

3 participants