2
\$\begingroup\$

I'm an amateur coder writing a geometry library in Python, and would like some feedback regarding my implementation of a class for 3D Vectors.

My priority is to have a really friendly API for beginners who are just starting out writing code, to build it with as few dependencies as possible (no numpy or scipy), and for it to be compatible with at least Python 2.6, hopefully as early as 2.4.

EDIT #1: I really just want general feedback and thought an experienced coder might see glaring errors relating to performance or object hashing and comparison.

EDIT #2: @Winston raised a good criticism about my goal to not use dependencies, so I'll explain this goal in more detail. My primary audience for this library is beginners who may be using Python or writing code for the first time. This may be the first library they've ever had to put on sys.path and import, and there is a decent chance they will utilize the package to script a GUI application, and therefore within a non-standard implementation of Python, such as IronPython. I want to minimize the complexity of using the library for first time users. But yeah, I totally love the array of fantastic packages in Python, but ideally, I just don't want the first 3rd party package someone uses to have additional dependencies, and if I have to, it should come with "batteries included"

These goals are more important than performance to me, but if I can improve performance (or anything else) without sacrificing these bigger priorities, that would be great.

class Vector3D(object):
    """A 3D vector object. Intended to basically be an api wrapper 
       around a tuple of 3 float values. It would allow people to 
       reset different coordinates, and to treat the Vector like a 
       list or even a dictionary, but at heart it would be a tuple 
       of floats.
    """

    def __init__(self, x=0.0, y=0.0, z=0.0):
        for arg in (x, y, z):
            if not isinstance(arg, numbers.Number):
                raise TypeError
        # coords is the essence of the data structure. It's immutable and
        # iterable
        self.coords = (x, y, z)
    # defining x, y, and z like this to allow for the coords to remain a
    # non-mutable iterable.
    @property
    def x(self):
        return self[0]
    @x.setter
    def x(self, number):
        self.coords = (number, self[1], self[2])
    @property
    def y(self):
        return self[1]
    @y.setter
    def y(self, number):
        self.coords = (self[0], number, self[2])
    @property
    def z(self):
        return self[2]
    @z.setter
    def z(self, number):
        self.coords = (self[0], self[1], number)

    @property
    def length(self):
        """get the vector length / amplitude
            >>> v = Vector3D(0.0, 2.0, 1.0)
            >>> v.length
            2.2360679774997898
        """
        # iterate through the coordinates, square each, and return the root of
        # the sum
        return math.sqrt(sum(n**2 for n in self))

    @length.setter
    def length(self, number):
        """set the vector amplitude
            >>> v = Vector3D(0.0, 2.0, 1.0)
            >>> v.length
            2.2360679774997898
            >>> v.length = -3.689
            >>> v
            Vector3D(-0.0, -3.2995419076, -1.6497709538)
        """
        # depends on normalized() and __mult__
        # create a vector as long as the number
        v = self.normalized() * number
        # copy it
        self.match(v)


    def normalize(self):
        """edits vector in place to amplitude 1.0 and then returns self
            >>> v
            Vector3D(-0.0, -3.2995419076, -1.6497709538)
            >>> v.normalize()
            Vector3D(-0.0, -0.894427191, -0.4472135955)
            >>> v
            Vector3D(-0.0, -0.894427191, -0.4472135955)
        """
        # depends on normalized and match
        self.match(self.normalized())
        return self

    def normalized(self):
        """just returns the normalized version of self without editing self in
        place.
            >>> v.normalized()
            Vector3D(0.0, 0.894427191, 0.4472135955)
            >>> v
            Vector3D(0.0, 3.2995419076, 1.6497709538)
        """
        # think how important float accuracy is here!
        if isRoughlyZero(sum(n**2 for n in self)):
            raise ZeroDivisionError
        else:
            return self * (1 / self.length)

    def match(self, other):
        """sets the vector to something, either another vector,
        a dictionary, or an iterable.
        If an iterable, it ignores everything
        beyond the first 3 items.
        If a dictionary, it only uses keys 'x','y', and 'z'
            >>> v
            Vector3D(0.0, 3.2995419076, 1.6497709538)
            >>> v.match({'x':2.0, 'y':1.0, 'z':2.2})
            >>> v
            Vector3D(2.0, 1.0, 2.2)
        """
        # this basically just makes a new vector and uses it's coordinates to
        # reset the coordinates of this one.
        if isinstance(other, Vector3D):
            self.coords = other.coords
        elif isinstance(other, dict):
            self.coords = (other['x'], other['y'], other['z'])
        else: # assume it is some other iterable
            self.coords = tuple(other[:3])

    def asList(self):
        """return vector as a list"""
        return [c for c in self]

    def asDict(self):
        """return dictionary representation of the vector"""
        return dict( zip( list('xyz'), self.coords ) )

    def __getitem__(self, key):
        """Treats the vector as a tuple or dict for indexes and slicing.
            >>> v
            Vector3D(2.0, 1.0, 2.2)
            >>> v[0]
            2.0
            >>> v[-1]
            2.2000000000000002
            >>> v[:2]
            (2.0, 1.0)
            >>> v['y']
            1.0
        """
        # key index
        if isinstance(key, int):
            return self.coords[key]
        # dictionary
        elif key in ('x','y','z'):
            return self.asDict()[key]
        # slicing
        elif isinstance(key, type(slice(1))):
            return self.coords.__getitem__(key)
        else:
            raise KeyError

    def __setitem__(self, key, value):
        """Treats the vector as a list or dictionary for setting values.
            >>> v
            Vector3D(0.0, 1.20747670785, 2.4149534157)
            >>> v[0] = 5
            >>> v
            Vector3D(5, 1.20747670785, 2.4149534157)
            >>> v['z'] = 60.0
            >>> v
            Vector3D(5, 1.20747670785, 60.0)
        """
        if not isinstance(value, numbers.Number):
            raise ValueError
        if key in ('x','y','z'):
            d = self.asDict()
            d.__setitem__(key, value)
            self.match(d)
        elif key in (0,1,2):
            l = self.asList()
            l.__setitem__(key, value)
            self.match(l)
        else:
            raise KeyError

    def __iter__(self):
        """For iterating, the vectors coordinates are represented as a tuple."""
        return self.coords.__iter__()

    ## Time for some math

    def dot(self, other):
        """Gets the dot product of this vector and another.
            >>> v
            Vector3D(5, 1.20747670785, 60.0)
            >>> v1
            Vector3D(0.0, 2.0, 1.0)
            >>> v1.dot(v)
            62.41495341569977
        """
        return sum((p[0] * p[1]) for p in zip(self, other))

    def cross(self, other):
        """Gets the cross product between two vectors
            >>> v
            Vector3D(5, 1.20747670785, 60.0)
            >>> v1
            Vector3D(0.0, 2.0, 1.0)
            >>> v1.cross(v)
            Vector3D(118.792523292, 5.0, -10.0)
        """
        # I hope I did this right
        x = (self[1] * other[2]) - (self[2] * other[1])
        y = (self[2] * other[0]) - (self[0] * other[2])
        z = (self[0] * other[1]) - (self[1] * other[0])
        return Vector3D(x, y, z)

    def __add__(self, other):
        """we want to add single numbers as a way of changing the length of the
        vector, while it would be nice to be able to do vector addition with
        other vectors.
            >>> from core import Vector3D
            >>> # test add
            ... v = Vector3D(0.0, 1.0, 2.0)
            >>> v1 = v + 1
            >>> v1
            Vector3D(0.0, 1.4472135955, 2.894427191)
            >>> v1.length - v.length
            0.99999999999999956
            >>> v1 + v
            Vector3D(0.0, 2.4472135955, 4.894427191)
        """
        if isinstance(other, numbers.Number):
            # then add to the length of the vector
            # multiply the number by the normalized self, and then
            # add the multiplied vector to self
            return self.normalized() * other + self

        elif isinstance(other, Vector3D):
            # add all the coordinates together
            # there are probably more efficient ways to do this
            return Vector3D(*(sum(p) for p in zip(self, other)))
        else:
            raise NotImplementedError

    def __sub__(self, other):
        """Subtract a vector or number
            >>> v2 = Vector3D(-4.0, 1.2, 3.5)
            >>> v1 = Vector3D(2.0, 1.1, 0.0)
            >>> v2 - v1
            Vector3D(-6.0, 0.1, 3.5)
        """
        return self.__add__(other * -1)

    def __mul__(self, other):
        """if with a number, then scalar multiplication of the vector,
            if with a Vector, then dot product, I guess for now, because
            the asterisk looks more like a dot than an X.
            >>> v2 = Vector3D(-4.0, 1.2, 3.5)
            >>> v1 = Vector3D(2.0, 1.1, 0.0)
            >>> v2 * 1.25
            Vector3D(-5.0, 1.5, 4.375)
            >>> v2 * v1 #dot product
            -6.6799999999999997
        """
        if isinstance(other, numbers.Number):
            # scalar multiplication for numbers
            return Vector3D( *((n * other) for n in self))

        elif isinstance(other, Vector3D):
            # dot product for other vectors
            return self.dot(other)

    def __hash__(self):
        """This method provides a hashing value that is the same hashing value
        returned by the vector's coordinate tuple. This allows for testing for
        equality between vectors and tuples, as well as between vectors.

        Two vector instances (a and b) with the same coordinates would return True
        when compared for equality: a == b, a behavior that I would love to
        have, and which would seem very intuitive.

        They would also return true when compared for equality with a tuple
        equivalent to their coordinates. My hope is that this will greatly aid
        in filtering duplicate points where necessary - something I presume
        many geometry algorithms will need to look out for.

        I'm not sure it is a bad idea, but I intend this class to basically be a
        tuple of floats wrapped with additional functionality.
        """
        return self.coords.__hash__()

    def __repr__(self):
        return 'Vector3D%s' % self.coords

Some specific questions as prompts, but I'm mostly just looking for general review:

  1. Is it foolish to implement a __hash__ method that returns true between the object and a tuple contained in one of it's attributes? I thought this method would be useful for creating sets of vectors, and comparing for equality between vectors. Will it have some unforeseen consequences in terms of comparison?
  2. Are there an glaring performance mistakes that I'm making?
  3. Is there anything else that you would warn me about, or opportunities I'm missing?
\$\endgroup\$
2
\$\begingroup\$
        for arg in (x, y, z):
            if not isinstance(arg, numbers.Number):
                raise TypeError

Don't check types. Just assume that the user passed in the correct types. I am serious. It'll waste time every time you make a Vector3D class.

        # coords is the essence of the data structure. It's immutable and
        # iterable
        self.coords = (x, y, z)

What are you trying to accomplish with this tuple? This class doesn't act like a tuple, because you can modify the coordinates, so there is no reason to be using a tuple.

    @property
    def x(self):
        return self[0]
    @x.setter
    def x(self, number):
        self.coords = (number, self[1], self[2])

A Vector class should be immutable. You shouldn't have setters for x, y, or z. In python value objects like strings/integers/floats/etc are all immutable. If you break that pattern for your class, code will end up being confusing. The reason is because of code like this:

a = Vector3d()
b = a
a.x = 5
print b.x

a and b are the same object, so modifying one modifies the other. This tends to produce subtle bugs, hence objects that hold values like this are usually made immutable to prevent this.

to build it with as few dependencies as possible (no numpy or scipy)

Don't. That should (almost) never be a goal. Libraries like numpy are what makes python so great. If you avoid use a numpy when it is useful, you doom your library to be completely useless. Any sort of code that operations on data like this in python should be using numpy.

\$\endgroup\$
  • \$\begingroup\$ "Don't check types" - Thank you. I'll stop doing that. \$\endgroup\$ – BenjaminGolder Jun 5 '12 at 10:11
  • \$\begingroup\$ "A Vector class should be immutable. You shouldn't have setters for x, y, or z." - thanks, I've been going back and forth about this and how I've handled it. You put the inner debate to rest. It will be immutable. \$\endgroup\$ – BenjaminGolder Jun 5 '12 at 10:15
  • \$\begingroup\$ "Don't. That should (almost) never be a goal. Libraries like numpy are what makes python so great." - I strongly agreee that such libraries make python great, but I aim to focus on beginners as an audience, many of whom might be using the library to script a GUI application, so they may be using different implementations of Python (such as IronPython). It may also be the first library they've ever had to install and use. I admire how NetworkX handles this: written in pure python, but can be used with numpy if desired. \$\endgroup\$ – BenjaminGolder Jun 5 '12 at 10:20
  • \$\begingroup\$ Thank you for taking the time to review my code, I appreciate it a ton. Seriously. \$\endgroup\$ – BenjaminGolder Jun 5 '12 at 10:32
  • \$\begingroup\$ "Don't check types. Just assume that the user passed in the correct types." - One question: If I have methods that I want to respond differently to different input types, is it still a bad idea to use type checking in that context? \$\endgroup\$ – BenjaminGolder Jun 5 '12 at 10:34

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.