Default to float dtype?

[asnt]

Thanks for your efforts in creating this handy library.

In matrix33.create_from_axis_rotation(axis, theta, dtype=None), I was surprised by the fact that an int dtype for the axis is not automatically converted to float, which lead to unexpected results.

Example with float dtype => OK:

[GCC 8.1.1 20180531] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> import pyrr
>>> axis_float = (0, 1.0, 0)
>>> pyrr.matrix33.create_from_axis_rotation(axis_float, np.pi / 4)
array([[ 0.70710678,  0.        , -0.70710678],
       [ 0.        ,  1.        ,  0.        ],
       [ 0.70710678,  0.        ,  0.70710678]])

With int => KO:

>>> axis_int = (0, 1, 0)
>>> pyrr.matrix33.create_from_axis_rotation(axis_int, np.pi / 4)
array([[0, 0, 0],
       [0, 1, 0],
       [0, 0, 0]])
>>>

Or one can also pass dtype=float as parameter to force it:

>>> pyrr.matrix33.create_from_axis_rotation(axis_int, np.pi / 4, dtype=float)
array([[ 0.70710678,  0.        , -0.70710678],
       [ 0.        ,  1.        ,  0.        ],
       [ 0.70710678,  0.        ,  0.70710678]])

The input int dtype is forced there: https://github.com/adamlwgriffiths/Pyrr/blob/4bc98bc1abc72f860f97563e2a4459813d0bfd88/pyrr/matrix33.py#L103 It overrides the dtype of the array which is already float.

Wouldn't it make sense to return the float array by default in this situation?

Seems related to #35

[adamlwgriffiths]

The idea is to mimic the numpy behaviour as much as possible. If you pass in ints to a np.narray, you'll get an array of ints. Same thing here. The output you've provided is what is expected.

I can't recall the other issue entirely, but I believe it is because promoting types does not match what numpy does and can lead to the output remaining ints rather then being promoted to float when multiplied by a float.

[asnt]

To me, this behaviour is confusing. I would expect (0, 1, 0) or (0, 1., 0) to give me the same result because I'm looking at it from the mathematical point of view. Mathematically, a rotation matrix is defined with real values. Whether the programming language uses integers or floats as data type for the input doesn't matter so much, as the output only makes sense with floats (from this mathematical point of view). So, to me, the general case would be to output a float matrix. If truncation to integers is desired, one could pass dtype=int or call .astype(int) on the result. I am not saying this should be the case everywhere in the library. I think it could depend on the particular quantity computed.

Is there an example use case where the truncation to integers is desired?

[adamlwgriffiths]

There are valid uses for integer matrices (https://en.wikipedia.org/wiki/Integer_matrix). I do not use these but I am not here to dictate mathematics.

The idea is to mimic numpy's behaviour. Similar interface = lower barrier of entry = less to remember = less surprises.

If you make a matrix in numpy and pass in integers, you'll get an integer matrix. If you want to use floats, you should be explicit in your usage and pass in floats. Silent conversion is never a good thing. Sure, it can trip you up initially, but it means your code isn't right, I.e. you're mixing types. If you pass in mixed types, e.g. (0, 1., 0), I would argue that you've got a bug in your code. 1 and 1.0 are completely different things.

This is true for the creation of the structures. Once created and the dtypes are defined, the types will be coerced to the original dtypes.

Having classes act differently is not a good thing. As you have to remember how each one acts individually. And if you actually intend to use ints, you'll have to over-ride it. More code for no real benefit and more user surprises.

I appreciate the general use case is for rotation matrices, but there may be other uses that I haven't considered that are perfectly valid, and customising code to fit this use case can invalidate, or make harder, those use cases.

If you want to force floats, use the dtype=np.float or equivalent and you can then be lazy and drop the decimal point. But I would encourage you to be explicit and enter the values as floats as intended, otherwise you may end up with unintended behaviour in your code in other cases.

[asnt]

You are right that integer matrices exist in their own right. However, rotation matrices don't fall in this class. So this interface still seems unintuitive. I agree that if all values are passed as floats that will work as expected. However, this is prone to error.

Another way to look at issue: create_from_axis_rotation takes an axis vector and an angle theta. Inside the function, theta is automatically converted to float by numpy because of numpy.sin(theta) and numpy.cos(theta). To be consistent with numpy.array, I think the dtype of the rotation matrix should be

the minimum type required to hold the objects

which is float because of sin(theta) and cos(theta).

From numpy.array:

dtype : data-type, optional

The desired data-type for the array. If not given, then the type will be determined as the minimum type required to hold the objects in the sequence. This argument can only be used to ‘upcast’ the array. For downcasting, use the .astype(t) method.

[adamlwgriffiths]

As long as nothing is removed by adding this, then I'm all for removing pit-falls.

It seems there is an np function that could be used to provide some hints as to whether to coerce or not. Something along the lines of:

def ...(...):
    # assuming arr is a numpy array
    if not np.issubdtype(arr.dtype, np.floating) and dtype is None:
        dtype = float
    <rest of function>

It seems that numpy defaults to float64 regardless of platform byte-width, so the appropriate default dtype would be python float which maps to np.float_ and in turn np.float64. (Today-I-Learned)

I don't have any time to implement and test at the moment (I know it's pretty trivial =P). If you want to do a PR that would be awesome, but again being time poor I'd delegate the approval to one of the other contributors.

[asnt]

Do you mean that we should output a np.float32 dtype if axis is of this type, otherwise fall back to float?

[adamlwgriffiths]

No, stick to standard float definition, which is float64. The code snippet above should be pretty much all that is needed.

[asnt]

In the above snippet, what does the arr refer to? Is it the axis or the resulting rotation matrix?

[adamlwgriffiths]

The relevant argument for the function in question. Ie, depends on the context =P. For create_from_axis_rotation it would be axis, for create_from_quaternion, it would be quat, etc. I may be wrong, but it looks the easiest way to do so.