8
\$\begingroup\$

I create a 2dVector class to use a position coordinates in a pygame application and maybe in the future as part of a physics engine. Does this implementation has any huge downsides? Is the type hinting ok?

import math
import random
from numbers import Real
from typing import Union, Sequence, cast

VectorType = Sequence[Real]


class Vector(VectorType):
    x: Real
    y: Real
    rotation: Real
    magnitude: Real

    def __init__(self, x: Union[VectorType, Real], y: Real = None) -> None:
        if y is None:
            x, y = cast(VectorType, x)
        self.x, self.y = cast(Real, x), y

    def __repr__(self):
        return f"{self.__class__.__name__}{self.x, self.y}"

    def __len__(self) -> int:
        return 2

    def __getitem__(self, i: int) -> Real:
        return (self.x, self.y)[i]

    def __add__(self, other: VectorType):
        return Vector(self.x + other[0], self.y + other[1])

    def __radd__(self, other):
        return Vector(self.x + other[0], self.y + other[1])

    def __iadd__(self, other: VectorType):
        self.x += other[0]
        self.y += other[1]
        return self

    def __sub__(self, other: VectorType):
        return Vector(self.x - other[0], self.y - other[1])

    def __rsub__(self, other: VectorType):
        return Vector(other[0] - self.x, other[1] - self.y)

    def __isub__(self, other: VectorType):
        self.x -= other[0]
        self.y -= other[1]
        return self

    def __mul__(self, other: Real) -> Vector:
        return Vector(self.x * other, self.y * other)

    def __rmul__(self, other: Real):
        return Vector(self.x * other, self.y * other)

    def __matmul__(self, other: VectorType):
        return Vector(self.x * other[0], self.y * other[1])

    def __rmatmul__(self, other):
        return Vector(other[0] * self.x, other[1] * self.y)

    def __truediv__(self, other: Union[VectorType, Real]) -> Vector:
        if isinstance(other, Sequence):
            return Vector(self.x / other[0], self.y / other[1])
        return Vector(self.x / other, self.y / other)

    def __rtruediv__(self, other: VectorType) -> Vector:
        self.x /= other
        self.y /= other
        return self

    def __round__(self, n=None) -> Vector:
        return Vector(round(self.x, n), round(self.x, n))

    def change_rotation(self, rotation: float) -> Vector:
        v = Vector(self.x, self.y)
        v.rotation += rotation
        return v

    @classmethod
    def random(cls, x_range=(0, 1), y_range=(0, 1)):
        return cls(random.uniform(*x_range), random.uniform(*y_range))

    @classmethod
    def from_polar(cls, rotation: Real, magnitude: Real):
        return Vector(math.cos(rotation) * magnitude, math.sin(rotation) * magnitude)

    def normalize(self):
        f = self.magnitude
        self.x *= f
        self.y *= f
        return self

    @property
    def magnitude(self) -> Real:
        return (self.x * self.x + self.y * self.y) ** 0.5

    @magnitude.setter
    def magnitude(self, value: Real):
        f = value / self.magnitude
        self.x *= f
        self.y *= f

    @property
    def rotation(self):
        return math.atan2(self.x, self.y)

    @rotation.setter
    def rotation(self, value):
        m = self.magnitude
        self.x = math.sin(value)
        self.y = math.cos(value)
        self.magnitude = m

    @property
    def rounded(self):
        return round(self.x), round(self.y)

Here is the code on github (with a few additions)

\$\endgroup\$
  • \$\begingroup\$ Vector(round(self.x, n), round(self.x, n)) typo for Vector(round(self.x, n), round(self.y, n)) ? \$\endgroup\$ – Jean-François Fabre Apr 30 '18 at 20:07
  • \$\begingroup\$ @Jean-FrançoisFabre Yes, glad you pointed that out. Would have been a pretty hard to catch bug. \$\endgroup\$ – MegaIng Apr 30 '18 at 20:08
  • \$\begingroup\$ also in rotation you're assigning m to the magnitude, then assign it the other way round: it achieves nothing \$\endgroup\$ – Jean-François Fabre Apr 30 '18 at 20:08
  • 1
    \$\begingroup\$ pretty nice code, that said \$\endgroup\$ – Jean-François Fabre Apr 30 '18 at 20:10
  • \$\begingroup\$ @Jean-FrançoisFabre yes it does. It will set self.x and self.y to the correct values. (Multiply them with magnitude) , because self.magnitude = m invokes the property setter. \$\endgroup\$ – MegaIng Apr 30 '18 at 20:11
2
\$\begingroup\$
  1. There are no docstrings.

  2. I would suggest reconsidering the mutability of the vectors. The advantages of mutability are:

    a. You get to reuse the original storage for the original vector when you make a change, avoiding a couple of memory management operations.

    b. You save a couple of characters when writing some update operations, for example you can write v.normalize() instead of, say, v = v.normalized().

    But the disadvantage of mutability is that it is harder to reason about operations. If you see a function that takes some vectors, let's say

    def collide(p: VectorType, q: VectorType, r: Real) -> Bool:
        "Return True if p and q are separated by no more than r."
    

    is it safe to pass p=sprite.position, or do you need to make a copy? With mutability, then for all you know, the function is implemented like this:

    p -= q
    return p.magnitude <= r
    

    So in a world where vectors are mutable, you have to worry about this possibility, and perhaps document for each function which arguments get mutated and which don't. If vectors are immutable, then all this is unnecessary and reasoning about the behaviour of code is easier.

    Immutability would avoid the need to implement the __iadd__ and __isub__ methods. Also, you could inherit from tuple and save some memory. See this vector class on github for some implementation ideas.

  3. The __truediv__ method has different behaviour based on the type of the argument. This is inconsistent with the behaviour of __mul__ and __rmul__ and __rtruediv__, none of which have this behaviour. Also, dispatching based on the type makes it harder to read and reason about code. If you know that v is a vector and then you read the expression v / a, you can't immediately deduce that a must be a scalar as you could in the case of v * a.

    I guess that you added the variant operation because of some use case. But nonetheless I would recommend giving the variant operation a different method name, for example scaled.

  4. In __radd__ the operations should be reversed, with the other values on the left (just in case they have unusual implementations of __add__ method):

    def __radd__(self, other):
        return Vector(other[0] + self.x, other[1] + self.y)
    

    Similarly for __rmul__.

  5. There's no checking that the other vector has the right length. There's nothing stopping me from writing:

    Vector(1, 2) + [3, 4, 5]
    

    which ought to be an error.

  6. In __repr__ I suggest writing type(self) rather than self.__class__, just as you would write len(s) rather than s.__len__().

  7. In from_polar you should call cls rather than Vector, to support subclasses.

  8. normalize has the operations the wrong way round: you need to divide by the magnitude, not multiply. But it would be simpler to write self.magnitude = 1.

  9. In magnitude it would be clearer to call the argument length or magnitude rather than value.

  10. In magnitude, consider writing math.hypot(self.x, self.y).

  11. In rotation it would be clearer to call the argument theta or angle rather than value.

\$\endgroup\$
  • \$\begingroup\$ Do you think I need to add docstrings to all methods? Even __add__ or __getitem__? \$\endgroup\$ – MegaIng May 1 '18 at 14:24
  • \$\begingroup\$ I wouldn't write docstrings for methods that are already adequately documented in the Python language reference. Unless there's something special or surprising about your implementation. \$\endgroup\$ – Gareth Rees May 1 '18 at 14:29
  • \$\begingroup\$ Added suggestion 1 and 4-11 to github. To 2: I want to keep the mutability. It is quite useful. To 3: I want to keep the simple division by a scalar or a vector. I no it is a mess, I already thoughtabout overwriting __divmod__ and returning result, None, but that is better in my opinion. Any ideas? \$\endgroup\$ – MegaIng May 1 '18 at 14:47
  • \$\begingroup\$ I mean overwriting __divmod__ isn't better \$\endgroup\$ – MegaIng May 1 '18 at 14:58
  • \$\begingroup\$ The specification for __divmod__ says: "The __divmod__ method should be the equivalent to using __floordiv__ and __mod__; it should not be related to __truediv__." So overriding this method with something related to __truediv__ has the potential to lead to confusion and error. I suggest using another method name, for example scaled or scaled_down. \$\endgroup\$ – Gareth Rees May 3 '18 at 13:15

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.