0

So I am running into some trouble trying to debug this piece of code. I have a list of numbers, say [4,5,7,3,5,2,3] and I am required to find the two points who are closest together, so in this case, 3 and 3 since their difference is zero. However, it is not returning the correct output. It works if a number is not repeated in a list, but won't work if the a number appears more than once.

    def closest1(num_list):
        if len(num_list) < 2:
            return (None, None)  
        else:
            diff = max(num_list), min(num_list)
            for element in num_list:
                for sec_element in num_list:
                    if sec_element == element:
                        continue
                    if abs(sec_element - element) < abs(diff[0] - diff[1]):
                        diff = sec_element, element
        return diff
PythonSOS
  • 339
  • 5
  • 16
  • have you tried sorting the list first – Keatinge Apr 27 '16 at 00:25
  • `[x for x in num_list if num_list.count(x) > 1]` will return the repetitions. If there are none, you can continue to sort the list and find the differences. – jDo Apr 27 '16 at 00:32
  • There is another pair which has distance 0, (5,5) in that list. Is there a way to prefer this to (3,3) or it doesn't matter? – sal Apr 27 '16 at 00:39
  • Oh yeah sorry that was a typo... There's only supposed to be 3,3 and one 5 – PythonSOS Apr 27 '16 at 01:07

5 Answers5

0

I think that your problem is that when you find equal number you run a continue statement. In that place the condition should be the position of each number. If the position is equal you should skip the number, BUT if the position is different and the number is equal is a valid case.

In the other side, you don't need to look for the minimum neither maximum number and after that compute the longest distance. Just start with the difference of the first two numbers to better performance.

Here is the code fixed:

def closest1(num_list):
        if len(num_list) < 2:
            return (None, None)
        else:
            num1 = num_list[0]
            num2 = num_list[1]
            diff = abs(num1 - num2)
            if diff == 0:  # Better case ever! You compute almost nothing! :D
                return num1, num2
            # enumerates(list) gives you the pair (position, value) for each item of the list.
            for p1, element in enumerate(num_list):
                for p2, sec_element in enumerate(num_list):
                    if p1 == p2:  # Compare positions, not values ;)
                        continue  # Here is the big fix!
                    if abs(sec_element - element) < abs(diff):
                        diff = sec_element - element
                        num1 = element
                        num2 = sec_element
                        if diff == 0:  # Great case! Don't have to compute all the list! :)
                            return num1, num2
        return num1, num2

if __name__ == '__main__':
    print("Should be 0,1 and it is %s,%s" % closest1(range(10)))
    print("Should be 4,4 and it is %s,%s" % closest1([4,5,6,7,8,6,4,2]))

You can run it directly.

NOTE: This code is just for educational purpose, there are more performant ways of doing it.

Hamlett
  • 403
  • 6
  • 22
0

You can use itertools to first provide all the combinations of the elements in the list by 2, and then calculate their distance. Last step would be to just return the min of such list, so your function could be rewritten as:

import itertools
def closest1(num_list):
    return min([(abs(x[0]-x[1]),x) for x in itertools.combinations(num_list, 2)])

Returns (0, (3,3)) See in action here: https://eval.in/560204

sal
  • 3,515
  • 1
  • 10
  • 21
0

Maybe thats what you want?

if you want the numbers that are closest i think this is the solution.

if you want the smallest diffrence return diff instead.

def closest(num_list):
    tmp = (None, None)
    diff = max(num_list) - min(num_list)
    for i in range(len(num_list)):
        for j in range(len(num_list)):
            if i != j and diff >= abs(num_list[i] - num_list[j]) :
                tmp = (num_list[i], num_list[j])
                diff = abs(num_list[i] - num_list[j])
    return tmp
print(closest([4,5,7,3,5,2,3]))
Svaris
  • 11
  • 3
0

Only one loop is necessary if you sort first:

def closest1(num_list):
    num_list = sorted(num_list)
    diff = num_list[0] - num_list[-1]
    diff_dict = {"num1":diff, "num2":diff, "diff":diff} 
    for pos, val in enumerate(num_list[:-1]):
        diff = abs(num_list[pos+1] - val)
        if diff < diff_dict["diff"]:
            diff_dict = {"num1":num_list[pos+1], "num2":val, "diff":diff}
    return diff_dict
jDo
  • 3,962
  • 1
  • 11
  • 30
  • Yeah we're supposed to use nested for loops for this function... The second function uses sort – PythonSOS Apr 27 '16 at 01:08
  • @PythonSOS Ok, I forget that these questions are homework sometimes. Nested loops are often avoided in real code but if it's a requirement here, I think Svaris' or Hamlett's solutions are the best so far. – jDo Apr 27 '16 at 01:19
0

You must use a range iteration so the condition if sec_element == element: won't confuse the comparison of tow different elements with the same value with the comparison of an element with itself.

Also you don't have to loop over all elements in the second loop. Here is a variation of your code:

def closest1(num_list):
    if len(num_list) < 2:
        return (None, None)  

    a, b = num_list[0], num_list[1]
    diff = b - a
    for i in range(len(num_list)):
        for j in range(i):
            new_diff = num_list[i] - num_list[j]
            if abs(new_diff) < abs(diff):
                diff = new_diff
                a, b = num_list[i], num_list[j]
    return a, b
nino_701
  • 672
  • 3
  • 13