5
\$\begingroup\$

I've just started a course that will require knowledge and experience with Python for future projects, which I don't have, so I thought I'd give it a go to familiarize myself with it and get some feedback. I got a decent summary of the language's main features from a 2 hour long overview video (https://www.youtube.com/watch?v=H1elmMBnykA), tried a few small things on my own, then decided to move on to something a bit more interesting.

As the title indicates, the code consists of two classes: Complex, which represents complex numbers, and ComplexTest which is a sequence of unit tests for the Complex class. I am aware that Python supports complex numbers natively. I should also mention that all the unit tests from ComplexTest run properly and pass.

I'm interested in comment about literally any parts of my code, since this is my first time writing Python code. Any and all feedback is welcome!

Finally, one point which irked me a bit was the apparent clash between Python 2 and Python 3, which often made me unsure whether the way I was implementing things was "correct" or not from the Python 3 perspective (which is the one I'm targeting).

I also really miss my semicolons and curly brackets :(

ccomplex.py

from numbers import Number
import math


class Complex:
    def __init__(self, re=0, im=0):
        self._re = re
        self._im = im

    def __eq__(self, other):
        if isinstance(other, Complex):
            return self.re == other.re and self.im == other.im
        else:
            raise TypeError("The argument should be an instance of Complex")

    def __neg__(self):
        return Complex(-self.re, -self.im)

    def __add__(self, other):
        if isinstance(other, Complex):
            return Complex(self.re + other.re, self.im + other.im)
        else:
            raise TypeError("The argument should be an instance of Complex")

    def __sub__(self, other):
        if isinstance(other, Complex):
            return self + (-other)
        else:
            raise TypeError("The argument should be an instance of Complex")

    def __mul__(self, other):
        if isinstance(other, Complex):
            a = self.re * other.re - self.im * other.im
            b = self.re * other.im + self.im * other.re
            return Complex(a, b)
        elif isinstance(other, Number):
            return Complex(self.re * other, self.im * other)
        else:
            raise TypeError(
                "The argument should be an instance of Complex or Number")

    def __rmul__(self, other):
        return self * other

    def __truediv__(self, other):
        if isinstance(other, Complex):
            if self.re == 0 and self.im == 0:
                return Complex(0, 0)

            if other.re == 0 and other.im == 0:
                raise ValueError("The argument should be different from zero")

            return (self * other.conj()) / other.mod_squared()
        elif isinstance(other, Number):
            return Complex(self.re / other, self.im / other)
        else:
            raise TypeError(
                "The argument should be an instance of Complex or Number")

    def __rtruediv__(self, other):
        if isinstance(other, Complex):
            if other.re == 0 and other.im == 0:
                return Complex(0, 0)

            if self.re == 0 and self.im == 0:
                raise ValueError("The argument should be different from zero")

            return (other * self.conj()) / self.mod_squared()
        elif isinstance(other, Number):
            return Complex(other, 0) / self
        else:
            raise TypeError(
                "The argument should be an instance of Complex or Number")

    def conj(self):
        return Complex(self.re, -self.im)

    def mod_squared(self):
        return self.re * self.re + self.im * self.im

    def mod(self):
        return math.sqrt(self.mod_squared())

    def arg(self):
        return math.atan2(self.im, self.re)

    @property
    def re(self):
        return self._re

    @re.setter
    def re(self, value):
        self._re = value

    @property
    def im(self):
        return self._im

    @im.setter
    def im(self, value):
        self._im = value

    def __str__(self):
        op = "+" if self.im >= 0 else "-"
        return "{} {} {}i".format(self.re, op, abs(self.im))

complexTest.py

from ccomplex import Complex
import math
import unittest


class ComplexTest(unittest.TestCase):
    def test_equality(self):
        self.assertTrue(Complex(2, 2) == Complex(2, 2))

    def test_inequality(self):
        self.assertFalse(Complex(1, 1) == Complex(2, 2))

    def test_equality_raises_type_exception(self):
        with self.assertRaises(TypeError):
            z = Complex(2, 2) == "Not A Complex"

    def test_negation(self):
        self.assertEqual(-Complex(4, 4), Complex(-4, -4))

    def test_sum(self):
        z = Complex(2, 2)
        self.assertEqual(z + z, Complex(4, 4))

    def test_difference(self):
        z = Complex(4, 4)
        self.assertEqual(z - Complex(2, 2), Complex(2, 2))

    def test_complex_product(self):
        z1 = Complex(4, 4)
        z2 = Complex(2, 2)
        self.assertEqual(z1 * z2, Complex(0, 16))

    def test_product_raises_type_exception(self):
        with self.assertRaises(TypeError):
            z = Complex(2, 2) * "Not A Complex"

    def test_left_real_product(self):
        z = Complex(2, 2)
        self.assertEqual(z * 2, Complex(4, 4))

    def test_right_real_product(self):
        z = Complex(2, 2)
        self.assertEqual(2 * z, Complex(4, 4))

    def test_complex_division(self):
        z1 = Complex(4, 4)
        z2 = Complex(2, 2)
        self.assertEqual(z1 / z2, Complex(2, 0))

    def test_division_raises_type_exception(self):
        with self.assertRaises(TypeError):
            z = Complex(2, 2) / "Not A Complex"

    def test_complex_division_raises_zero_division_exception(self):
        with self.assertRaises(ValueError):
            z = Complex(2, 2) / Complex(0, 0)

    def test_real_division_raises_zero_division_exception(self):
        with self.assertRaises(ZeroDivisionError):
            z = Complex(2, 2) / 0

    def test_left_real_division(self):
        z = Complex(4, 4)
        self.assertEqual(z / 2, Complex(2, 2))

    def test_right_real_division(self):
        z = Complex(2, 2)
        self.assertEqual(2 / z, Complex(0.5, -0.5))

    def test_conjugate(self):
        z = Complex(2, 2)
        self.assertEqual(z.conj(), Complex(2, -2))

    def test_mod_squared(self):
        z = Complex(2, 2)
        self.assertAlmostEqual(z.mod_squared(), 8, delta=10e-16)

    def test_mod(self):
        z = Complex(2, 2)
        self.assertAlmostEqual(z.mod(), 2 * math.sqrt(2), delta=10e-16)

    def test_arg(self):
        z = Complex(2, 2)
        self.assertAlmostEqual(z.arg(), math.pi / 4, delta=10e-16)


if __name__ == '__main__':
    unittest.main(verbosity=2)
\$\endgroup\$
  • \$\begingroup\$ Just a note: Code Review is a low bandwidth site. Unlike the Stack Overflow (which tends to be more of a fastest-gun-in-the-west site), code reviews should be slower, and more methodical. As such, you might want to wait more than 24 hours after posting your question before accepting an answer; questions with accepted answers tend to receive less attention. As Linus's law states, "given enough eyeballs, all bugs are shallow", so you don't want to discourage those eyeballs. \$\endgroup\$ – AJNeufeld Sep 17 at 21:56
3
\$\begingroup\$

Looks pretty good.

I see you implemented modulus in mod. It's also called absolute value, and that's the name Python uses. If you implement __abs__, then Python's abs function can use it. Then abs(Complex(3, 4)) would give you 5.0. Just like Python's own abs(3 + 4j) does.

Another useful one is __bool__, which lets you declare zero as false, as is standard in Python. Currently you fail this (i.e., it does get printed):

if Complex(0, 0):
    print('this should not get printed')

You could then also use that twice inside your __truediv__ method. Like if not self:.

The (in)equality test could be extended. For example, I'd expect Complex(3) == 3 to give me True, not crash. And then your tests inside __truediv__ could alternatively be like if self == 0:.

You can have a look at what Python's own complex numbers have:

>>> for name in dir(1j):
        print(name)

__abs__
__add__
__bool__
__class__
__delattr__
__dir__
__divmod__
__doc__
__eq__
__float__
...
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ For example, I'd expect Complex(3) == 3 to give me True, not crash -- good point, I wish I'd thought of that. And the simplified self == 0 checks too, thanks! Just to be sure I understand correctly, if I implement __bool__ then I won't need to write if self == 0, instead I could simply write if self, correct? \$\endgroup\$ – cliesens Sep 17 at 12:22
  • 1
    \$\begingroup\$ @cliesens Not if self but if not self. Unless you implement __bool__ in the opposite way it should be :-). But I guess this shows that if self == 0 would be clearer. \$\endgroup\$ – superb rain Sep 17 at 12:39
  • \$\begingroup\$ Right, I missed that; woops! I just realized it while implementing __ne__ (by the way, should I implement it whenever I also implement __eq__?) and some additional tests for the new methods I've just written. Thanks again! \$\endgroup\$ – cliesens Sep 17 at 12:50
  • 1
    \$\begingroup\$ @cliesens Good question. I think it's not necessary to implement __ne__ here. The default works. See the answers to this question, but ignore the accepted answer since it's outdated. \$\endgroup\$ – superb rain Sep 17 at 15:03
3
\$\begingroup\$

Value Objects

The following shows what most users would consider unexpected behaviour:

from ccomplex import Complex

a = Complex(5, 4) + Complex(3)
b = a
a.re = -a.re
print(b)  # "-8 + 4i"

Values are usually considered to be immutable. Since Python uses objects to represent values, and objects have identity which can be shared, the best practice is to use immutable objects when creating what are normally considered values. This looks like it is modifying a string:

a = "Hello"
a += " world"

But since str does not implement the __iadd__ operator, what Python actually does is interpret the statement as a = a + " world", which evaluates the expression a + " world", and assigns the result (a new object) to a. The identity of a changes as the a += ... statement is executed, since a different object is stored in that variable.

>>> a = "hello"
>>> id(a)
1966355478512
>>> a += " world"
>>> id(a)
1966350779120
>>> 

Removing the @re.setter and @im.setter methods would change your Complex class to be publicly immutable. While that is a good start, nothing prevents someone from manipulating the internals directly, like a._re = 7.

The easiest way to make this class truly immutable is to inherit from an immutable base. Assuming you're using at least Python 3.7:

from typing import NamedTuple

class Complex(NamedTuple):
    re: float
    im: float = 0

    def __eq__(self, other):
        if isinstance(other, Complex):
            return self.re == other.re and self.im == other.im
        else:
            return NotImplemented

     # ... etc ...

The NamedTuple base class automatically creates the constructor for you, so Complex(2, 3) produces your 2 + 3i complex value. If no value is provided for im, such as Complex(2), the default of 0 is used for im.

If you wanted to change the re or im value, you must create a new object.

a = Complex(-8, a.im)

or, using NamedTuple._replace:

a = a._replace(re=-8)

Not Implemented

The astute reader will notice return NotImplemented in the above. This is a magic singleton, which is a signal to Python to try alternatives. For instance, a == b could fallback on not a.__neq__(b), b.__eq__(a), or even not b.__ne__(a).

Consider: you may not know about a Matrix class, but it might know about your Complex class. If someone does cmplx * matrix, if your __mul__ function raises TypeError, it's game over. If instead NotImplemented is returned, then Matrix.__rmul__ can be tried, which might work.

See NotImplemented and Implementing the arithmetic operations

rtruediv

When evaluating a / b, first is tried a.__truediv__(b). If that fails (was not defined or returns NotImplemented), b.__rtruediv__(a) may be tried.

class Complex:
    ...
    def __rtruediv__(self, other):
        if isinstance(other, Complex):
            ...
        ...

Why would isinstance(other, Complex) ever be true? It would mean both that self is a Complex (since we're in Complex.__rtruediv__), and other is a Complex (since isinstance says so in this scenario). But if that is the case, we're doing Complex() / Complex(), so then __truediv__ should have been used, and __rtruediv__ wouldn't even need to be considered.

Division by Zero

Why does Complex(2, 2) / 0 raise a ZeroDivisionError where as Complex(2, 2) / Complex(0, 0) raises a ValueError? Shouldn't it be raising a ZeroDivisionError?

Your test name test_complex_division_raises_zero_division_exception doesn't match the with self.assertRaises(ValueError) condition, which suggests you knew what it should have raised, and discovered the error, but changed the test to match the condition that was raised, instead of raising the correct exception.

| improve this answer | |
\$\endgroup\$

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.