-
Notifications
You must be signed in to change notification settings - Fork 54
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
Default to float dtype? #80
Comments
The idea is to mimic the numpy behaviour as much as possible. 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. |
To me, this behaviour is confusing. I would expect Is there an example use case where the truncation to integers is desired? |
There are valid uses for integer matrices (https://en.wikipedia.org/wiki/Integer_matrix). The idea is to mimic numpy's behaviour. 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. 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. 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 |
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:
which is float because of From numpy.array:
|
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.
It seems that numpy defaults to float64 regardless of platform byte-width, so the appropriate default dtype would be python 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. |
Do you mean that we should output a |
No, stick to standard float definition, which is float64. |
In the above snippet, what does the |
The relevant argument for the function in question. Ie, depends on the context =P. |
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 anint
dtype for the axis is not automatically converted to float, which lead to unexpected results.Example with
float
dtype => OK:With
int
=> KO:Or one can also pass
dtype=float
as parameter to force it:The input
int
dtype is forced there:Pyrr/pyrr/matrix33.py
Line 103 in 4bc98bc
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
The text was updated successfully, but these errors were encountered: