fixed quaternion_from_euler modifying the input in some cases

[salihmarangoz]

Same as #236 and I still wonder why this is still not fixed.

[salihmarangoz]

@tfoote I think quaternion_from_euler is not designed for array inputs. So, we can assume that inputs are scalars. This PR should fix the problem. You can examine the problem with this example case:

#! /usr/bin/env python

import tf
import numpy as np

roll, pitch, yaw = 0.1, 0.2, 0.3
roll = np.array(roll)
pitch = np.array(pitch)
yaw = np.array(yaw)

print(roll, pitch, yaw)
q1 = tf.transformations.quaternion_from_euler(roll, pitch, yaw)
print(roll, pitch, yaw)

The output should be:

0.1 0.2 0.3
0.1 0.2 0.3

but it is

0.1 0.2 0.3
0.05 0.1 0.15

[salihmarangoz]

Avoiding in-place math operators (+=, -=, /=, *=) would be a better fix.

[peci1]

Thanks for the PR! Would you be able to add a unit test that fails without the fix and passes with it?

[salihmarangoz]

@peci1 I can write tests against this type of problem like below. This can check if the output is a view of an input arg, and if the input args are modified or not. My only concern is that the last commit was created on 5 Jan 2022... I think the devs are focused on ROS2.

def modify_numpy_array(a):
    with numpy.nditer(a, op_flags=['readwrite']) as it:
       for x in it:
           x[...] = 2 * x

...

    def test_transformations_translation_matrix_arg_immutability(self):
        v = numpy.random.rand(3)
        v.flags.writeable = False
        M = tf.transformations.translation_matrix(v)
        modify_numpy_array(M)

    def test_transformations_translation_from_matrix_arg_immutability(self):
        v0 = numpy.random.rand(4,4)
        v0.flags.writeable = False
        v1 = tf.transformations.translation_from_matrix(v0)
        modify_numpy_array(v1)

    def test_transformations_reflection_matrix_arg_immutability(self):
        v0 = numpy.random.rand(4)
        v0[3] = 1.0
        v1 = numpy.random.rand(3)
        v0.flags.writeable = False
        v1.flags.writeable = False
        R = tf.transformations.reflection_matrix(v0, v1)
        modify_numpy_array(R)

    def test_transformations_reflection_from_matrix_arg_immutability(self):
        v0 = numpy.random.rand(3) - 0.5
        v1 = numpy.random.rand(3) - 0.5
        M0 = tf.transformations.reflection_matrix(v0, v1)
        M0.flags.writeable = False
        point, normal = tf.transformations.reflection_from_matrix(M0)
        modify_numpy_array(point)
        modify_numpy_array(normal)

    def test_transformations_rotation_matrix_arg_immutability(self):
        angle = numpy.array((numpy.random.rand() - 0.5) * (2*numpy.pi))
        direc = numpy.random.rand(3) - 0.5
        point = numpy.random.rand(3) - 0.5
        angle.flags.writeable = False
        direc.flags.writeable = False
        point.flags.writeable = False
        R0 = tf.transformations.rotation_matrix(angle, direc, point)
        modify_numpy_array(R0)

[peci1]

last commit was created on 5 Jan 2022... I think the devs are focused on ROS2.

You're right about that, but this issue seems to me like important enough to get their attention. I can provide a review for the PR, which could also help (though I'm not a maintainer).

I don't understand the proposed tests. I'd expect you'd test the quaternion_from_euler function calls.

[salihmarangoz]

I don't understand the proposed tests. I'd expect you'd test the quaternion_from_euler function calls.

This is a generalized test for the same type of problem where input arguments are modified. Because I thought adding a small test just to prevent this specific bug doesn't make sense. If it is to have their attention I can do that. Alternatively, I can do the same thing for all tf.transformations.* functions.

[peci1]

I think the base for getting the PR merged is testing the particular function you're editing.

A generalized test doesn't make sense to me as it only proves known behavior of numpy.

Testing all tf.transformations functions would be good, but I wouldn't say it's required.

[tfoote]

A unit test that demonstrates the failure and then is fixed afterwards would be a good first step. This element of the code is an external library and in general when working with external libraries that are imported we don't want to diverge from the upstream ones so that behavior is consistent. We've run into problems with good patches being rejected upstream and then we end up unintentially forking and making more problems for our users. So to that end it would be also great to submit this upstream as well for review first.

[peci1]

Upstream already has the fix (and also the unit test): https://github.com/matthew-brett/transforms3d/pull/48 .

[tfoote]

Please consider submitting this patch to @DLu too for tf_transformations in ros 2 distros.

https://github.com/DLu/tf_transformations/commit/369fd1c6617c6ec4906ba14599d3572d07ab27d0#diff-99c393c0d67fc7556c1bda99b89b3bb00ef90480b58059f783c7d3f40dd6c609R993-R995

[salihmarangoz]

Please consider submitting this patch to @DLu too for tf_transformations in ros 2 distros.

DLu/tf_transformations@369fd1c#diff-99c393c0d67fc7556c1bda99b89b3bb00ef90480b58059f783c7d3f40dd6c609R993-R995

@tfoote looks like it is replaced with transforms3d: https://github.com/DLu/tf_transformations/commit/18c5497dd35f22f3ab20976e4e156062fc808ac8#diff-99c393c0d67fc7556c1bda99b89b3bb00ef90480b58059f783c7d3f40dd6c609L993

Also, @peci1 showed that it is already fixed in transforms3d. I think there is nothing left to do.