6
\$\begingroup\$

The goal of the function is to find out how to get from one angle to another angle. Two things are involved: the amount to move by and the direction (+ means turn right, - means turn left). The most the angle will have to move by is 180deg. The function is rather short. I wanted to avoid using and trig functions or pi. I want this as fast as I can while maintaining readability.

import pandas as pd
import numpy as np

def get_wind_dir_difd(dir_1, dir_2):
    """
    Figures out the shortest way to get from dir_1 to dir_2. Positive nunber go clockwise, negative numbers go counter clockwise.
    NOTE: The direction 0 and 360 are the same.
    :param dir_1: int. The direction of the first wind.
    :param dir_2: The direction of the second wind.
    :return: int. The what to add (can be a negative number too) to the first wind dir to get to the second wind dir.
    """
    diff = dir_2 - dir_1

    if diff > 180:  # the highest diff can be is 180
        diff = 360 - diff

    elif diff < -180:  # the lowest diff can be is -180
        diff = 360 + diff

    if dir_2 > dir_1 and (dir_2 - dir_1 > 180):  # ensures the correct sign is used when dir_2 is larger and the difference betwene the two is more than 180, EX 10, 310 or 40, 360
        diff = -diff

    return diff

# IGNORE WHAT IS BELOW, THAT IS JUST TO TEST

get_wind_dir_dif_vec = np.vectorize(get_wind_dir_difd)

df = pd.read_csv('test.csv')

df['diff'] = get_wind_dir_dif_vec(df['val1'], df['val2'])

for i in range(len(df)):
    row = df.iloc[i]
    correct_or_not = row['diff'] == row['what_diff_should_be']
    if correct_or_not:
        correct_or_not = ''
    else:
        correct_or_not = 'X'
    df.set_value(i, ' bad_answer ', correct_or_not)

print(df)

To give you a better idea of the program

    val1  val2  diff  what_diff_should_be  bad_answer 
0    120    30   -90                  -90             
1    340    20    40                   40             
2     20   340   -40                  -40             
3    310    10    60                   60             
4     10   310   -60                  -60             
5      0   300   -60                  -60             
6    300     0    60                   60             
7    190   180   -10                  -10             
8    180   190    10                   10             
9    200   220    20                   20             
10   220   200   -20                  -20             
11    10   190   180                  180             
12   360     0     0                    0             
13     0   360     0                    0             
14   340    20    40                   40             
15   350     0    10                   10             
16   350   360    10                   10             
17    40   360   -40                  -40             
18   180     0  -180                 -180             
19     0   180   180                  180             

This is the csv I tested it on.

val1,val2,diff,what_diff_should_be
120,30,,-90
340,20,,40
20,340,,-40
310,10,,60
10,310,,-60
0,300,,-60
300,0,,60
190,180,,-10
180,190,,10
200,220,,20
220,200,,-20
10,190,,180
360,0,,0
0,360,,0
340,20,,40
350,0,,10
350,360,,10
40,360,,-40
180,0,,-180
0,180,,180
\$\endgroup\$
  • \$\begingroup\$ What about abs(dir_1 - dir_2) * ((dir_2 > dir_1) - (dir_1 > dir_2))? \$\endgroup\$ – alexyorke Dec 4 '19 at 4:20
  • \$\begingroup\$ @alexyorke won't that always return a positive value? Some of the output will be negative. \$\endgroup\$ – Vader Dec 4 '19 at 4:23
  • \$\begingroup\$ it returns negative values (I tried it with a few examples in your answer.) The ((dir_2 > dir_1) - (dir_1 > dir_2)) is a convoluted way to get the sign of the number; if it's positive it will be 1, negative it will be -1 (and thus multiply the answer by that.) \$\endgroup\$ – alexyorke Dec 4 '19 at 4:26
  • 3
    \$\begingroup\$ return (dir_2 - dir_1 + 180) % 360 - 180 That works in all example cases except the last, where it returns -180 instead of 180, which is arguably equivalent. \$\endgroup\$ – AJNeufeld Dec 4 '19 at 4:39
  • 1
    \$\begingroup\$ I'm not sure how that helps with interpolation; regardless, you should edit that requirement so it's part of the question (comments can be easily lost). \$\endgroup\$ – Toby Speight Dec 4 '19 at 15:39
6
\$\begingroup\$
  • Since you seem to be targeting Sphinx for your documentation you should either:

    1. Specify your parameter types correctly in the docstring.

      :param int dir_1: The direction of the first wind.
      

      Or

      :param dir_1: The direction of the first wind.
      :type dir_1: int
      
    2. Get Sphinx to do this for you from PEP 484 type hints. (Python 3.5+ notation)

      def get_wind_dir_difd(dir_1: int, dir_2: int) -> int:
          """
          :param dir_1: The direction of the first wind.
          """
      
  • Your docstring is rather hard to read as they just go off forever. Limit the length of them so it's easier to read them when developing.

  • Take advantage of Sphinx NOTE: should use .. note::.
  • Your first two comments are pretty meh. The third should really be before the if so it's easier to read.
  • Check your spelling - number, between.
def get_wind_dir_difd(dir_1: int, dir_2: int) -> int:
    """
    Figures out the shortest way to get from :code:`dir_1` to :code:`dir_2`.
    Positive number go clockwise, negative numbers go counter clockwise.

    .. note::

        The direction 0 and 360 are the same.

    :param dir_1: Direction of the first wind.
    :param dir_2: Direction of the second wind.
    :return: What to add to :code:`dir_1` to get :code:`dir_2`.
    """
    diff = dir_2 - dir_1
    if diff > 180:
        diff = 360 - diff
    elif diff < -180:
        diff = 360 + diff

    # Ensures the correct sign is used when dir_2 is larger
    # and the difference between the two is more than 180.
    # EX 10, 310 or 40, 360
    if dir_2 > dir_1 and (dir_2 - dir_1 > 180):
        diff = -diff

    return diff

  • dir_2 > dir_1 is always true when dir_2 > dir_1 + 180.
  • dir_2 > dir_1 and (dir_2 - dir_1 > 180) is the same as diff > 180. So you can easily merge the ifs together.
def get_wind_dir_difd(dir_1: int, dir_2: int) -> int:
    diff = dir_2 - dir_1
    if diff > 180:
        diff -= 360
    elif diff < -180:
        diff += 360
    return diff
\$\endgroup\$
  • \$\begingroup\$ Looks like my biggest mistake was doing diff = 360 - diff instead of diff -= 360 \$\endgroup\$ – Vader Dec 4 '19 at 15:14
8
\$\begingroup\$

The other reviews give some good formatting advice so I'll concentrate solely on the function itself. I would probably write this instead:

def dir_diff(dir1, dir2):
    diff = (dir2 - dir1) % 360
    return diff if diff <= 180 else diff - 360

The way Python's % operator works, it assures that we always have a positive number from 0 to 360 for diff in the first line. Then we only need to adjust if the value is greater than 180. Note also that unlike all of the other versions (including the original), this version will also continue to work properly even if we pass in values such as 900, -92 or 725, 9900.

Better yet, use AJNeufeld's comment or twalberg's equivalent:

return 180 - (dir1 - dir2 + 180) % 360

In another comment, the OP said:

Later I will use this function to interpolate missing data. So, if dir_1 is lower than dir_2 like it is with (0, 180) then the answer should be positive because 90 (90 b/c +180/2) would be the middle value and not -90. Hope that helps.

This requirement doesn't make sense to me because interpolation between two diametrically opposite points on a circle can legitimately be interpreted as either midpoint. However, if it's really a requirement, we can combine like so:

def Edward2(dir1, dir2):
    diff = (dir2 - dir1 + 180) % 360 - 180
    return -diff if diff == -180 and dir1 < dir2 else diff

If anyone cares, here is the test code I used:

import csv

def orig(dir_1, dir_2):
    diff = dir_2 - dir_1

    if diff > 180:
        diff = 360 - diff

    elif diff < -180:
        diff = 360 + diff

    if dir_2 > dir_1 and (dir_2 - dir_1 > 180):
        diff = -diff

    return diff

def Edward1(dir1, dir2):
    diff = (dir2 - dir1) % 360
    return diff if diff <= 180 else diff - 360

def twalberg(dir1, dir2):
    return 180 - (dir1 - dir2 + 180) % 360

def AJneufeld(dir1, dir2):
    return (dir2 - dir1 + 180) % 360 - 180

def Edward2(dir1, dir2):
    diff = (dir2 - dir1 + 180) % 360 - 180
    return -diff if diff == -180 and dir1 < dir2 else diff

if __name__=="__main__":
    green = "\x1b[32m"
    red = "\x1b[91m"
    white = "\x1b[37m"
    algo = [orig, Edward1, twalberg, AJneufeld, Edward2]


    def testAll(d1, d2, desired):
        print('{:-6} {:-6} {:-6}'.format(d1, d2, desired), end='')
        for fn in algo:
            calc = fn(int(d1),int(d2))
            color = [green, red][calc != desired]
            print('{}{:>10}{}'.format(color, calc, white), end='')
        print()

    print('  dir1   dir2 desired', end='')
    for fn in algo:
        print(f'{fn.__name__:>10}', end='')
    print()
    with open('input.csv') as csvfile:
        rowreader = csv.reader(csvfile)
        for row in rowreader:
            testAll(int(row[0]), int(row[1]), int(row[2]))

Here is the test data I used, with the format dir1,dir2,desired.

input.csv

120,30,-90
340,20,40
20,340,-40
310,10,60
10,310,-60
0,300,-60
300,0,60
190,180,-10
180,190,10
200,220,20
220,200,-20
10,190,180
360,0,0
0,360,0
340,20,40
350,0,10
350,360,10
40,360,-40
180,0,-180
0,180,180
1,181,180
359,179,-180
180,0,-180
181,1,-180
179,359,180
90,270,180
270,90,-180
-90,-270,-180
-270,-90,180
0,360,0
360,0,0
180,-180,0
-180,-180,0
-180,180,0
180,180,0
900,-92,88
725,9900,175
725,9905,180
9905,725,-180

Results

With the test cases shown, only the Edward2 function gets all results correct. I'm using Python 3.7.5.

\$\endgroup\$
  • 1
    \$\begingroup\$ or even return 180 - (dir1 - dir2 + 180) % 360 and skip the extra variable.... \$\endgroup\$ – twalberg Dec 4 '19 at 18:16
  • \$\begingroup\$ Yes, and that’s also equivalent to AJNeufeld’s comment which I had overlooked until a moment ago. \$\endgroup\$ – Edward Dec 4 '19 at 18:20
  • \$\begingroup\$ Well, it's nearly equivalent, but it gets the one corner case right that his didn't... \$\endgroup\$ – twalberg Dec 4 '19 at 18:45
  • \$\begingroup\$ @twalberg They just get different ones "wrong": try 0,180 and 180,0 for instance. \$\endgroup\$ – Edward Dec 4 '19 at 19:24
  • \$\begingroup\$ Valid point, but it does at least get all the test cases listed by the OP... When the two directions are 180 apart, either +180 or -180 are valid, I guess... \$\endgroup\$ – twalberg Dec 4 '19 at 19:53
5
\$\begingroup\$

Consolidation and unified conditionals

  • the primary difference dir_2 - dir_1 occurs twice in the initial approach: once at the 1st assignment and another time - in the last if condition.
    Thus, Extract variable technique can be applied to avoid repetition:

    prime_diff = final_diff = dir_2 - dir_1
    

    where prime_diff is the primary/initial difference and the final_diff is derivative final difference that may or may not be modified due to further conditions

  • the 1st condition if diff > 180 and last sub-check (dir_2 - dir_1 > 180) are essentially the same expression, thus, can beforehand be extracted into a variable to avoid repetition:

    gt180 = prime_diff > 180    # greater than 180 (gt)
    
  • the crucial if ... elif conditional containing 2 statements can be unified to a single or conditional with 1 consolidated statement:

    if gt180 or prime_diff < -180: 
    
  • the 1st sub-check dir_2 > dir_1 in the last conditional if dir_2 > dir_1 and (dir_2 - dir_1 > 180): is redundant because dir_2 - dir_1 > 180 will be True only when dir_2 is greater than dir_1 in any way.


The final get_wind_dir_difd function is simplified to the following:

def get_wind_dir_difd(dir_1, dir_2):
    """
    Figures out the shortest way to get from dir_1 to dir_2. Positive nunber go clockwise, negative numbers go counter clockwise.
    NOTE: The direction 0 and 360 are the same.
    :param dir_1: int. The direction of the first wind.
    :param dir_2: The direction of the second wind.
    :return: int. The what to add (can be a negative number too) to the first wind dir to get to the second wind dir.
    """
    prime_diff = final_diff = dir_2 - dir_1
    gt180 = prime_diff > 180    # greater than 180 (gt)

    if gt180 or prime_diff < -180:
        final_diff = 360 - abs(prime_diff)
        if gt180:
            final_diff = -final_diff

    return final_diff

Test results:

    val1  val2  diff  what_diff_should_be  bad_answer 
0    120    30   -90                  -90             
1    340    20    40                   40             
2     20   340   -40                  -40             
3    310    10    60                   60             
4     10   310   -60                  -60             
5      0   300   -60                  -60             
6    300     0    60                   60             
7    190   180   -10                  -10             
8    180   190    10                   10             
9    200   220    20                   20             
10   220   200   -20                  -20             
11    10   190   180                  180             
12   360     0     0                    0             
13     0   360     0                    0             
14   340    20    40                   40             
15   350     0    10                   10             
16   350   360    10                   10             
17    40   360   -40                  -40             
18   180     0  -180                 -180             
19     0   180   180                  180        
\$\endgroup\$
  • 3
    \$\begingroup\$ To me this isn't simplified, it's more confusing. Also to some this may read as just an alternate solution. Could you explain what have you changed? \$\endgroup\$ – Peilonrayz Dec 4 '19 at 8:52
  • \$\begingroup\$ Thank you for the update. The curse of knowlege is that changes that are fairly obvious to you, may not be obvious to others. \$\endgroup\$ – Peilonrayz Dec 4 '19 at 9:45
  • \$\begingroup\$ @Peilonrayz, Ok, I've added a detailed explanation, for now that should be easy to understand and follow \$\endgroup\$ – RomanPerekhrest Dec 4 '19 at 9:47
  • \$\begingroup\$ Yes, it should be. Sorry I think my second remark has caused some confusion here. \$\endgroup\$ – Peilonrayz Dec 4 '19 at 9:49

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.