Improve array support #8

MrBlenny · 2022-05-16T05:04:07Z

Thanks so much for your work here. This PR aims to improve support for arrays and sequences.

Example 1 - array.array[?]:

sensor_msgs/Image.msg has property uint8[] data

This generates the following in _image.py

 @property
    def data(self):
        """Message field 'data'."""
        return self._data

    @data.setter
    def data(self, value):
        if isinstance(value, array.array):
            assert value.typecode == 'B', \
                "The 'data' array.array() must have the type code of 'B'"
            self._data = value
            return
        if __debug__:
            from collections.abc import Sequence
            from collections.abc import Set
            from collections import UserList
            from collections import UserString
            assert \
                ((isinstance(value, Sequence) or
                  isinstance(value, Set) or
                  isinstance(value, UserList)) and
                 not isinstance(value, str) and
                 not isinstance(value, UserString) and
                 all(isinstance(v, int) for v in value) and
                 all(val >= 0 and val < 256 for val in value)), \
                "The 'data' field must be a set or sequence and each value of type 'int' and each unsigned integer in [0, 255]"
        self._data = array.array('B', value)

As such, the pyi should be:

    @property
    def data(self) -> array.array[int]: ...
    @data.setter
    def data(self, value: typing.Union[typing.Sequence[int], array.array[int]]) -> None: ...

Example 2 - np.ndarray:

sensor_msgs/Imu.msg has property float64[9] orientation_covariance

This generates the following in _imu.py

  @property
    def orientation_covariance(self):
        """Message field 'orientation_covariance'."""
        return self._orientation_covariance

    @orientation_covariance.setter
    def orientation_covariance(self, value):
        if isinstance(value, numpy.ndarray):
            assert value.dtype == numpy.float64, \
                "The 'orientation_covariance' numpy.ndarray() must have the dtype of 'numpy.float64'"
            assert value.size == 9, \
                "The 'orientation_covariance' numpy.ndarray() must have a size of 9"
            self._orientation_covariance = value
            return
        if __debug__:
            from collections.abc import Sequence
            from collections.abc import Set
            from collections import UserList
            from collections import UserString
            assert \
                ((isinstance(value, Sequence) or
                  isinstance(value, Set) or
                  isinstance(value, UserList)) and
                 not isinstance(value, str) and
                 not isinstance(value, UserString) and
                 len(value) == 9 and
                 all(isinstance(v, float) for v in value) and
                 True), \
                "The 'orientation_covariance' field must be a set or sequence with length 9 and each value of type 'float'"
        self._orientation_covariance = numpy.array(value, dtype=numpy.float64)

As such, the pyi should be:

    @property
    def orientation_covariance(self) -> np.ndarray: ...
    @orientation_covariance.setter
    def orientation_covariance(self, value: typing.Union[typing.Sequence[float], np.ndarray]) -> None: ...

This PR corrects the pyi to be as described above. Note that array and numpy is now imported into EVERY pyi file, even if it is not used.

bonprosoft

Thank you very much! 😃
I left some comments, please take a look!

rosidl_generator_mypy/__init__.py

bonprosoft · 2022-05-17T15:59:06Z

FYI: I made a suggestion patch to this PR: 70dad0e
Please feel free to add anything! Thank you.

MrBlenny

Fantastic! I agee with all of that. I've made those changes.

Co-authored-by: Yuki Igarashi <[email protected]>

bonprosoft · 2022-05-18T05:11:24Z

rosidl_generator_mypy/__init__.py

+        type_annotation = to_type_annotation(
+            current_namespace, defined_classes, type_.value_type
+        )
+        return Annotation(


I read the specification.
https://design.ros2.org/articles/idl_interface_definition.html

According to the Type Mapping section on the document, shouldn't we check type_annotation.getter in SPECIAL_NESTED_BASIC_TYPES in the same way as AbstractSequence?

I'm not sure... I've tested this against most of the standard ros interfaces (eg sensor_msgs) and I think everything looked correct.
It could be prudent to write some tests which colcon build a bunch of messages and make assertions about the pyi code it generates but I'm not sure I'll have time to do that right now.

I referred to the rosidl_python implementation, and it actually follows the specification.

I think the member initialization part is helpful to understand this behavior:
https://github.com/ros2/rosidl_python/blob/6675823dd6123360912e233a830d1edab89c7651/rosidl_generator_py/resource/_msg.py.em#L320-L332

I've also tested with the following message definition and got the same result that I expected.

Definition

int64[] integer_array int64[4] integer_fixed_array std_msgs/Header[] header_array std_msgs/Header[4] header_fixed_array

integer_array

@integer_array.setter def integer_array(self, value): if isinstance(value, array.array): assert value.typecode == 'q', \ "The 'integer_array' array.array() must have the type code of 'q'" self._integer_array = value return if __debug__: from collections.abc import Sequence from collections.abc import Set from collections import UserList from collections import UserString assert \ ((isinstance(value, Sequence) or isinstance(value, Set) or isinstance(value, UserList)) and not isinstance(value, str) and not isinstance(value, UserString) and all(isinstance(v, int) for v in value) and all(val >= -9223372036854775808 and val < 9223372036854775808 for val in value)), \ "The 'integer_array' field must be a set or sequence and each value of type 'int' and each integer in [-9223372036854775808, 9223372036854775807]" self._integer_array = array.array('q', value)

integer_fixed_array

@integer_fixed_array.setter def integer_fixed_array(self, value): if isinstance(value, numpy.ndarray): assert value.dtype == numpy.int64, \ "The 'integer_fixed_array' numpy.ndarray() must have the dtype of 'numpy.int64'" assert value.size == 4, \ "The 'integer_fixed_array' numpy.ndarray() must have a size of 4" self._integer_fixed_array = value return if __debug__: from collections.abc import Sequence from collections.abc import Set from collections import UserList from collections import UserString assert \ ((isinstance(value, Sequence) or isinstance(value, Set) or isinstance(value, UserList)) and not isinstance(value, str) and not isinstance(value, UserString) and len(value) == 4 and all(isinstance(v, int) for v in value) and all(val >= -9223372036854775808 and val < 9223372036854775808 for val in value)), \ "The 'integer_fixed_array' field must be a set or sequence with length 4 and each value of type 'int' and each integer in [-9223372036854775808, 9223372036854775807]" self._integer_fixed_array = numpy.array(value, dtype=numpy.int64)

header_array

@header_array.setter def header_array(self, value): if __debug__: from std_msgs.msg import Header from collections.abc import Sequence from collections.abc import Set from collections import UserList from collections import UserString assert \ ((isinstance(value, Sequence) or isinstance(value, Set) or isinstance(value, UserList)) and not isinstance(value, str) and not isinstance(value, UserString) and all(isinstance(v, Header) for v in value) and True), \ "The 'header_array' field must be a set or sequence and each value of type 'Header'" self._header_array = value

header_fixed_array

@header_fixed_array.setter def header_fixed_array(self, value): if __debug__: from std_msgs.msg import Header from collections.abc import Sequence from collections.abc import Set from collections import UserList from collections import UserString assert \ ((isinstance(value, Sequence) or isinstance(value, Set) or isinstance(value, UserList)) and not isinstance(value, str) and not isinstance(value, UserString) and len(value) == 4 and all(isinstance(v, Header) for v in value) and True), \ "The 'header_fixed_array' field must be a set or sequence with length 4 and each value of type 'Header'" self._header_fixed_array = value

@MrBlenny I also encountered the same issue as you 😅
Actually, this PR is great, and It would be wonderful to merge this PR soon.
If you don't have time to update this PR, please let me know. I am happy to help you.

bonprosoft

Thank you for the update!
Sorry, I left another comment 🙇‍♂️

bonprosoft · 2022-05-25T06:15:47Z

@MrBlenny I left a comment. Let me kindly ping you just in case...

MrBlenny · 2022-05-31T03:16:40Z

Sorry @bonprosoft
Please see my latest commit

bonprosoft

LGTM! Thank you!

MrBlenny added 4 commits May 16, 2022 12:34

feat: attempt to add array support

0554ffb

feat: seems to be working

f7902ec

feat: add support for npdarray

5416fb0

refactor: tidy imports

af8a642

MrBlenny mentioned this pull request May 16, 2022

Python type annotation ros2/rosidl_python#157

Open

MrBlenny added 4 commits May 16, 2022 15:23

fix: don't use array.array for a sequence of non int/float

80659ff

fix: use SPECIAL_NESTED_BASIC_TYPES

a5fdf08

fix: constants

0db2973

fix: simplify SPECIAL_NESTED_BASIC_TYPES

3fd7c7d

bonprosoft reviewed May 17, 2022

View reviewed changes

rosidl_generator_mypy/__init__.py Outdated Show resolved Hide resolved

rosidl_generator_mypy/__init__.py Outdated Show resolved Hide resolved

rosidl_generator_mypy/__init__.py Outdated Show resolved Hide resolved

rosidl_generator_mypy/__init__.py Outdated Show resolved Hide resolved

Suggestion to array support

70dad0e

MrBlenny commented May 18, 2022

View reviewed changes

refactor: add suggestions

6fae84e

Co-authored-by: Yuki Igarashi <[email protected]>

bonprosoft reviewed May 18, 2022

View reviewed changes

fix: handled bounded lists of complex types as sequences

2359831

bonprosoft approved these changes Jun 2, 2022

View reviewed changes

bonprosoft merged commit ea8a376 into rospypi:master Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve array support #8

Improve array support #8

MrBlenny commented May 16, 2022 •

edited

Loading

bonprosoft left a comment

bonprosoft commented May 17, 2022 •

edited

Loading

MrBlenny left a comment •

edited

Loading

bonprosoft May 18, 2022

MrBlenny May 19, 2022

bonprosoft May 20, 2022 •

edited

Loading

bonprosoft May 24, 2022

bonprosoft left a comment

bonprosoft commented May 25, 2022

MrBlenny commented May 31, 2022

bonprosoft left a comment

Improve array support #8

Improve array support #8

Conversation

MrBlenny commented May 16, 2022 • edited Loading

Example 1 - array.array[?]:

Example 2 - np.ndarray:

bonprosoft left a comment

Choose a reason for hiding this comment

bonprosoft commented May 17, 2022 • edited Loading

MrBlenny left a comment • edited Loading

Choose a reason for hiding this comment

bonprosoft May 18, 2022

Choose a reason for hiding this comment

MrBlenny May 19, 2022

Choose a reason for hiding this comment

bonprosoft May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Definition

integer_array

integer_fixed_array

header_array

header_fixed_array

bonprosoft May 24, 2022

Choose a reason for hiding this comment

bonprosoft left a comment

Choose a reason for hiding this comment

bonprosoft commented May 25, 2022

MrBlenny commented May 31, 2022

bonprosoft left a comment

Choose a reason for hiding this comment

MrBlenny commented May 16, 2022 •

edited

Loading

bonprosoft commented May 17, 2022 •

edited

Loading

MrBlenny left a comment •

edited

Loading

bonprosoft May 20, 2022 •

edited

Loading