10
\$\begingroup\$

I wanted a convenient class to easy access the parameters inside. I am using a lot of math in my game; that's why a wanted to access them through x, y and z (for readability). And this is my outcome.

It is a class which lies on top of numpy array. It basically means that I can control a numpy array trough my class. And using x, y and z for accessing the parameters instead of using indices ([0], [1] or [2]).

For that I overwrite the built in functions.


Example:

a = numpy.array([1,1,1], dtype = "float32")
b = a[0]

to

a = Vector3(1,1,1)
b = a.x

So what I want to know is if this is a good way (I wanted a fast written implementation). Are there any performance issues and is it well written?

import numpy as np
from numbers import Number

class Vector3(object):
    def __init__(self, x = 0, y = 0, z = 0, dtype = "float32"):
        self._data = np.array([x,y,z], dtype = dtype)
        self.x = self._data[0]
        self.y = self._data[1]
        self.z = self._data[2]

    def __str__(self):
        return str("Vector3({0.x},{0.y},{0.z})".format(self))

    def __mul__(self, value):
        if isinstance(value, type(self)):
            result = self._data * value._data
        elif isinstance(value, Number):
            result = self._data * value
        return type(self)(x = result[0], y = result[1], z = result[2])

    def __rmul__(self, value):
        return self.__mul__(value) # Kommutativgesetz/commutative

    def __add__(self, value):
        if isinstance(value, type(self)):
            result = self._data + value._data
        elif isinstance(value, Number):
            result = self._data + value
        return type(self)(x = result[0], y = result[1], z = result[2])

    def __radd__(self, value):
        return self.__add__(value) # Kommutativgesetz/commutative

    def __sub__(self, value):
        if isinstance(value, type(self)):
            result = self._data - value._data
        elif isinstance(value, Number):
            result = self._data - value
        return type(self)(x = result[0], y = result[1], z = result[2])

    def __rsub__(self, value):
        if isinstance(value, type(self)):
            result = value._data - self._data
        elif isinstance(value, Number):
            result = value - self._data
        return type(self)(x = result[0], y = result[1], z = result[2])


#Test
if __name__ == "__main__":
    a = Vector3(5,5,5)
    b = Vector3(2,4,3)
    c = a * b
    d = 2 * a
    e = a * 2
    f = a + b
    g = a + 2
    h = 2 + a
    i = a - b
    j = 2 - a
    k = a - 2
    print(c,d,e,f,g,h,i,j,k, sep = "\n")
\$\endgroup\$
0
8
\$\begingroup\$

Your code and text mismatch. You state that you want to use x, y and z, but you don't actually do it. You only use them in the initializer, and then you use them as named parameters when you recreate your object when returning the value. So your code kind of obfuscates that you are working on a numpy array.

Simplify overridden functions

You could also simplify your code some as almost all of the functions does the same:

OPERATIONS = {
    "add" : numpy.add,
    "mul" : numpy.multiply,
    "sub" : numpy.subtract,
    ...
}

def _apply_operation(self, value, operation):

    if isinstance(value, type(self)):
        result = OPERATIONS[operation](value._data, self._data)
    elif isinstance(value, Number):
        result = OPERATIONS[operation](value, self._data)
    return type(self)(*result)

And this could be called like:

def __add__(self, value):
    return self._apply_operation(value, "add")

Another question is why do you use if ... elif, without any else. This does leave for potentially cases where neither of them hit, and this could lead to the result being undefined when returning.

If however you actually mean else instead of elif you simplify the calculation to:

result = OPERATIONS[operation](value._data if isinstance(value, type(self))
                                           else value,
                               self._data)

Or possibly even loose the result and do:

return type(self)(*OPERATIONS[operation](value._data if isinstance(value, type(self))
                                                     else value,
                                         self._data)

Consider adding/changing __repr__ and __str__

In Python it's normal to add spaces after commas, something your __str__ doesn't do. I would change the __str__ and add a __repr__ method like the following:

def __str__(self):
    return str('!r'.format(self))

def __repr__(self):
    return 'Vector3({0}, {1}, {2})'.format(*self._data)

Implement a better test scheme

Your tests are executed but you only have a visual test of them. A much better option would be to use doctest. This would allow you to write tests in the header of each function, and you could be sure they all works as expected.

Something like the following:

def __add__(self, value):
    """Adds the value into self, and returns result as Vector3.

    >>> Vector3(1, 2, 3) + Vector3(2, 4, 6)
    Vector3(3.0, 6.0, 9.0)

    >>> Vector3(1, 2, 3) + 5
    Vector3(6.0, 7.0, 8.0)
    """

    return self._apply_operation(value, "add")

And then in your main code you could simply do:

doctest.testmod()

If all tests pass, you'll see nothing, and it they fail, you'll see why the fail and both the expected and actual results.

Properties

If you want to use x, y and z as aliases into self._data you could use properties which changes self._data like the following:

@property
def x(self):
    """Property x is first element of vector.

    >>> Vector3(10, 20, 30).x
    10.0
    """
    return self._data[0]

@x.setter
def x(self, value):
   self._data[0] = value

...

if __name__ == '__main__':
    doctest.testmod()

    a = Vector3(1, 2, 3)
    a.x = 10.0

    print('a = {}, a.z = {}'.format(a, a.x))

Which would output:

a = Vector3(10.0, 2.0, 3.0), a.z = 10.0

Do however remove the setting of self.x, self.y and self.z from __init__ so you don't have conflicting data variables.

\$\endgroup\$
5
  • \$\begingroup\$ Sorry if it was unclear. I thought it was obvious that I want to access a.x instead of using a[0]. I have written 'elif' because I want to add some cases during the development process. Your idea with that _apply_operation is realy good. \$\endgroup\$
    – Sens4
    Dec 18 '15 at 23:29
  • \$\begingroup\$ You are not accessing a.x so your text is not matching your code. In addition, as stated, not having an else is dangerous code. Try calling your code with something not a Vector3 and not a Number. \$\endgroup\$
    – holroy
    Dec 18 '15 at 23:34
  • \$\begingroup\$ here for example I am accessing a.x in my class str("Vector3({0.x},{0.y},{0.z})".format(self)). As I defined in my example above a represents an instance of my class Vector3. But this wasn't intent to be the general use case. I just wanted a class ... what I have already mentioned ... where I can easier accessing my parameters. \$\endgroup\$
    – Sens4
    Dec 18 '15 at 23:57
  • \$\begingroup\$ @Sens4, Using all of my suggestions you should have a lot simpler code, and still have the possibility to access it using a.x or similar. In addition you've added test verification which is a really good thing. \$\endgroup\$
    – holroy
    Dec 19 '15 at 0:24
  • \$\begingroup\$ Thanks for that, really detailed :) instead using a dictionary for the operations I'm using an Enum. So I dont have to remember the defined strings. In the _apply_operation method I am then calling operation.value(self._data, value._data) \$\endgroup\$
    – Sens4
    Dec 19 '15 at 0:32
8
\$\begingroup\$

No, I don't recommend doing that, because .x, .y, .z, and ._data are all independently mutable. For example, one might try to do:

a = Vector3(5, 5, 5)
a.x = 6

… and then a._data would be out of sync. (It's also possible to mutate the elements of a._data, but in that case you would obviously be doing something naughty with a private field, as indicated by the underscore prefix convention.)

So, you need to decide whether to make this class mutable or immutable.

If it's mutable, then you need to make x, y, and z into properties, with setters.

On the other hand, if you want to make the class immutable, then use a namedtuple (and also make the NumPy array read-only):

from collections import namedtuple

class Vector3(namedtuple('Vector3', ['x', 'y', 'z'])):
    def __new__(cls, x=0, y=0, z=0, dtype='float32'):
        self = super(Vector3, cls).__new__(cls, x, y, z)
        self._data = np.array([x, y, z], dtype=dtype)
        self._data.flags.writeable = False
        return self

    def __mul__(self, value):
        …

As a bonus, namedtuple gives you a somewhat reasonable implementation of __str__ for free.


Alternatively, just drop the entire class and use NumPy structured arrays to name your elements.

\$\endgroup\$
2
  • \$\begingroup\$ Okay so I want that the class is mutable. So I guess In every setter should be if it is e.g x, self._data[0] = x. In a getter for x , return self._data[0]. Thats accectly what I wanted to achieve. In the background a np array controlled by my Vector3!. \$\endgroup\$
    – Sens4
    Dec 19 '15 at 0:19
  • \$\begingroup\$ I accepted the other answer, because it was more detailed. \$\endgroup\$
    – Sens4
    Dec 19 '15 at 0:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.