image_geometry/cameramodels.py uses deprecated numpy.matrix subclass

[ScottMonaghan]

[!NOTE] I revised this issue with corrections and for clarity after I completed a close-reading of the ROS 2 Developer Guide,

I'd be happy to try to tackle solving and creating a PR for this issue, but I have some questions before I proceed.

Background

This issue arises from PendingDeprecationWarning reported in ros-perception/image_pipeline issue #572.

Issue Summary

image_geometry/cameramodels.py uses deprecated numpy.matrix subclass causing runtime PendingDeprecationWarning.

The use of numpy.matrix should be deprecated and/or replaced.

Details

PinholeCamera's nominally private members--K, D,R,P,full_K, and full_P are initialized as deprecated numpy.matrix objects (see below).

https://github.com/ros-perception/vision_opencv/blob/8b42d66f4946f4204387b30aabc8c2921e5d9f47/image_geometry/image_geometry/cameramodels.py#L6-L9

...

https://github.com/ros-perception/vision_opencv/blob/8b42d66f4946f4204387b30aabc8c2921e5d9f47/image_geometry/image_geometry/cameramodels.py#L32-L47

This will cause the warning during initialization every time the public API members below are called:

• PinholeCameraModel.fromCameraInfo(self,msg)
• StereoCameraModel.fromCameraInfo(self, left_msg, right_msg)

The following public members of PinholeCameraModel currently have a return type of numpy.matrix

• PinholeCameraModel.intrinsicMatrix(self)
• PinholeCameraModel.distortionCoeffs(self)
• PinholeCameraModel.rotationMatrix(self)
• PinholeCameraModel.projectionMatrix(self)
• PinholeCameraModel.fullIntrinsicMatrix(self)
• PinholeCameraModel.rotationMatrix(self)
• PinholeCameraModel.fullProjectionMatrix(self)

Suggested Acceptance Criteria for fix

[!NOTE] Updated 4/5/2024 based on feedback from @ijnek.

I'd like to do all these, but I've categorized them in order of priority: MUST HAVE, SHOULD HAVE, NICE TO HAVE.

MUST HAVE

• [ ] No supported API members result in a numpy.matrix() call
• [ ] No supported API members have a return type of numpy.matrix
• [ ] API documentation must be updated to reflect any changes
• [ ] Tests updated to reflect any changes & passing

SHOULD HAVE

• To prevent breaking changes to API:
  • [ ] A new supported API member that returns numpy.ndarray should be created for each currently supported API member that currently returns numpy.matrix
  • [ ] Existing API members that return numpy.matrix object generate a DeprecationWarning and suggest replacement method when called, and note changed return type of new methods
• General code cleanup
  • Update StereoCameraModel to only call public members of PinholeCameraModel, rather than private K,D,P,ect. directly.
  • Update to PEP8 Guidelines where possible

NICE TO HAVE

• [ ] New api methods should use snake_case to fit PEP8 Guidelines.
• [ ] Deprecate the rest of the camelCase public API methods with a warning and replace with snake_case
  • The justification here is that we don't know when this piece of code will be touched again and so it's my preference to bring it up to standard as much as possible.

@ijnek , I'm interested in your thoughts on the above. I'll take guidance from you on whether to do some all or none of the above. This would be my first ROS 2 contribution, so I'm excited to give this a shot.

[ijnek]

Hi @ScottMonaghan, sorry for the late reply and thanks for the nicely written issue.

Everything in general looks good, strategies for preventing API breakage are good too. Please use DeprecationWarning instead of PendingDeprecationWarning. Let's aim to deprecate in ROS 2 Jazzy, and completely remove in ROS 2 K-turtle.

To maintain functionality of deprecated API, I suggest updating underlying numpy data structure, and do a conversion to numpy.matrix upon request. The new methods can have the same name, but using snake case instead of camel case. Please add a note in the deprecation warning saying that the returned numpy data type of the new method is different.

[ScottMonaghan]

@ijnek, no problem with the delay. This past week was an object lesson in why we all need to treat our volunteer FOSS maintainers with gratitude.

Regarding time, I'm going to target getting a PR submitted by end of April;

Please let me know if that's too far out.

[ScottMonaghan]

@ijnek, with the merging of PR #527, this issue should be able to be closed.