3
\$\begingroup\$

I have just written this code that takes an initial coordinate and returns a new vector depending on a user-supplied range and bearing.

The calculation of the new position is done using Complex Numbers. I am sure there are easier ways to do this, but I wanted to play around with complex numbers. Any thoughts on these would be great.

The calculator is in a file called GenGeo, as this is where I keep lots of little modules like this that may be called again in other programs.

I have used **kwargs for a_e and a_n as these may not be required by the user. However, I have never really used *args and **kwargs before, so presume that I may have messed something up here.

This is the calculator module in GenGeo:

class CmO:
    """
    This class will contain specific code that is required to run a C-O calculation
    This includes range and bearing calc
    """

    def __init__(self, *args, **kwargs):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """
        self.a_e = kwargs.get('a_e', None)
        self.a_n = kwargs.get('a_n', None)

    def complex_randb(self, r, b):
        """
        An equation that using imaginary numbers to calculate the coordinates of a new
        point from a range and bearing of an old point
        :param r: range from original coordinate to new coordinate
        :param b: bearing from original coordinate to new coordinate
        """

        # -b is required as geodetic bearings are opposite to mathematical bearings
        t = complex(cos(radians(-b)), sin(radians(-b))) * complex(0, r)
        delta_easting = t.real
        delta_northing = t.imag

        if self.a_e and self.a_n is not False:

            new_easting = self.a_e + delta_easting
            new_northing = self.a_n + delta_northing

            return new_easting, new_northing

        else:
            return delta_easting, delta_northing

and this is the program that I have used to call it:

from GenGeo import CmO

initial_E = 100
initial_N = 100
input_range = 3
input_bearing = 310


a = CmO(a_e=initial_E, a_n=initial_N)
nE, nN = a.complex_randb(input_range, input_bearing)

print(f"The delta Easting and Northing are {round(nE, 3)}mE and {round(nN, 3)}mN")

Next project is writing something to calculate the range and bearing between two points

Any thoughts or improvements would be greatly appreciated!

\$\endgroup\$
  • 1
    \$\begingroup\$ Why are the initial easting and northing optional? Removing them from the second snippet results in an error. \$\endgroup\$ – Ted Brownlow Aug 10 at 0:12
  • \$\begingroup\$ @TedBrownlow - Very good point, oops. I have updated the code so that this is now actually an optional statement \$\endgroup\$ – JKRH Aug 10 at 0:32
3
\$\begingroup\$

Keyword-only parameters

You are not using **kwargs correctly. **kwargs should be used when you can accept any number of keywords, but you don't know what the keywords will be, such as dictionary creation. If you only accept 2 keyword parameters, you should list those keyword parameters explicitly:

    def __init__(self, *, a_e=None, a_n=None):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """

        self.a_e = a_e
        self.a_n = a_n

That * marks the end of positional parameters. a_e and a_n can be specified by keyword only. Since both default to None, both are optional. Any other keywords is reject with an error message, instead of silently being ignored.

Operator Precedence

        if self.a_e and self.a_n is not False:

This statement does not do what you think it does. is not is higher precedence than and, so it reads:

        if self.a_e and (self.a_n is not False):

thus, if a_n is never given as False, the result of is not will be True, and the and will always result in the truthiness of a_e only.

You probably intended the evaluation to be:

        if (self.a_e and self.a_n) is not False:

which tests the truthiness of both a_e and a_n. Sort of. There are very few ways of getting something for which is not False is not true out of that expression. The only way to get to the else expression is if a_e == False, or if a_e held a truthy value and a_n == False. Again, since if the values are not given, they are defaulted to None, and since None and None evaluates to None, and None is not False is a true statement, the if clause would be executed.

So you probably wanted to write:

        if self.a_e is not None and self.a_n is not None:

Why not Zero?

If you used 0 as the default for a_n and a_e, then

            new_easting = self.a_e + delta_easting
            new_northing = self.a_n + delta_northing

new_easting would simply become delta_easting and new_northing would become delta_northing, and you could always do the addition and return `new_easting, new_northing.

    def __init__(self, *, a_e=0, a_n=0):
        """
        :param a_e: initial easting
        :param a_n: initial northing
        """

        self.a_e = a_e
        self.a_n = a_n

    def complex_randb(self, r, b):
        """
        An equation that using imaginary numbers to calculate the coordinates of a new
        point from a range and bearing of an old point
        :param r: range from original coordinate to new coordinate
        :param b: bearing from original coordinate to new coordinate
        """

        # -b is required as geodetic bearings are opposite to mathematical bearings
        t = complex(cos(radians(-b)), sin(radians(-b))) * complex(0, r)
        delta_easting = t.real
        delta_northing = t.imag

        new_easting = self.a_e + delta_easting
        new_northing = self.a_n + delta_northing

        return new_easting, new_northing

Naming

Your parameter names a_e, a_n, r, and b are too short and cryptic. You should used easting northing, range, and bearing.

complex_randb is also confusing. The input is real, the output is real. The fact that complex numbers are internally used is an internal detail irrelevant to the caller. What is randb, some kind of random b? Oh, range_and_bearing! But it is not a range and bearing function, it is a new coordinate function:

    def new_coordinate(self, range, bearing):
        """Doc-string without the complex number internal detail mentioned"""
        ...

The class name CmO is also quite cryptic. Is that "C minus O" because it runs "C-O" calculations? You need a better class name.

| 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.