0

The basic way of implementing merge sort that is available everywhere is one in which we recursively create new left and right array for each time we perform the split -

https://www.geeksforgeeks.org/merge-sort/ --> [1]

I want to create only one auxiliary array like it is done in the below link-

https://www.coursera.org/learn/algorithms-part1/lecture/ARWDq/mergesort ->[2] - Minute (7 - 10)

The instructor clearly states that at 9:30 in the video - it's important to not create the auxiliary array in the recursive routine because that could lead to extensive cost of extra array creation. And you'll sometimes see Mergesort performing poorly because of that bug.

It does not create new arrays recursively.

Basically, I want to write the code mentioned in the coursera link in python

Here is what I have written so far ->

class merge_sort:

    def __init__(self, data):
        self.data = data

    aux_array = []


    def merge(array, aux_array, low, mid, high):

        for k in range(low, high):
            aux_array[k] = self.data[k]

        i = low
        j = mid + 1
        for k in range(low, high):
            if(i > mid):
                self.data[k] = aux_array[i]
                i = i +1
            elif(j > high):
                self.data[k] = aux_array[j]
                j = j +1
            elif(aux_array[i] < aux_array[j]):
                self.data[k] = aux_array[i]
                i = i +1
            else:
                self.data[k] = aux_array[j]
                j = j +1


    def mergesort(self, data, low, high):
        #high = len(data)
        mid = (high - low)//2
        mergesort(data, low, mid)
        mergesort(data, mid+1, high)
        merge(data, aux_array, low, mid, high)


    def start_sort(self):
        high = len(self.data) - 1
        self.mergesort(self.data, 0, high)


arr = [10,2,30,0,4]

arr1 = merge_sort(arr)
arr1.start_sort()
print(arr1)

I am currently getting following error -

TypeError: mergesort() takes 3 positional arguments but 4 were given

I have tried doing this as well -

@classmethod
    def mergesort(cls, data, low, high):
        #high = len(data)
        mid = (high - low)//2
        mergesort(data, low, mid)
        mergesort(data, mid+1, high)
        merge(data, aux_array, low, mid, high)

    def start_sort(self):
        high = len(self.data) - 1
        self.mergesort(self.data, 0, high)

In this case, I get following error -

NameError: name 'mergesort' is not defined
HARSHIT BAJPAI
  • 361
  • 2
  • 17
  • Is there a reason you want to sacrifice readability to do a merge sort like this? – Error - Syntactical Remorse Jun 05 '19 at 17:43
  • In the normal method(first link), two new arrays are being created each time the mergesort function is called. First two arrays are being created each of length half of the original. Then in the next call, two more arrays are being created each of 1/4th of original length. This goes on until we reach till 1 element only Whereas in the second method(second link), only one auxiliary array is being created at start and that is accessed throughout the code. So this method uses much less memory compared to the other. – HARSHIT BAJPAI Jun 05 '19 at 17:50
  • Actually memory used is very comparable. In example two you only use one list but you use a bigger one whereas normal solutions just do several small ones, plus doing rapid pushing and popping to a list isn't very efficient thus the `deque` object exists. Basically you are sacrificing lines and readability for a minimal amount of memory. – Error - Syntactical Remorse Jun 05 '19 at 17:52
  • How is memory comparable? In the first method, two arrays of half the original length are created first. Then, merge sort is applied to both of them. So 4 more arrays are created of size 1/4th the original array. This goes on and on until last and all the arrays are kept until they are merged. Isn't this taking more space than just creating another array of the size of the original array – HARSHIT BAJPAI Jun 05 '19 at 17:59
  • Where a = array size, x = number of array elements. The max space used with the first method will be 1a + 2*(1/2)a + 4*(1/4)a + ... + x*(1/x)a = 1a + 1a + 1a + ... + 1a = log2(2x)a Which can be fairly large. If this works the way I assume it works then it could take a considerably smaller amount of memory. – alexanderhurst Jun 05 '19 at 18:14
  • Did you mean to say, the method I am suggesting is taking less memory or the method in geeksforgeeks is taking less ? – HARSHIT BAJPAI Jun 05 '19 at 20:16
  • Also, if you have downvoted this question, can you please change that so that more people can come across this question and share their opinion – HARSHIT BAJPAI Jun 05 '19 at 20:27
  • The merge sort in the geeksforgeeks link is top down, while the merge sort in the Princeton link is bottom up. Most libraries use some variation of bottom up merge sort, like a hybrid insertion sort / merge sort (a bit faster, and O(1) stack space instead of O(log2(n)) stack space. The second example in [this answer](https://stackoverflow.com/questions/34844613/34845789#34845789) shows a bottom up merge sort that eliminates the copy step in merge. (b[] is the auxiliary array). – rcgldr Jun 05 '19 at 22:09
  • @alexanderhurst , Ok so I am currently doing this course Coursera algorithms part3 - https://www.coursera.org/learn/algorithms-part1/lecture/ARWDq/mergesort . If you go to week3's 3rd video and jump to 10 min, you can see the instructor saying that it's important to not create the auxiliary array in the recursive routine because that could lead to extensive cost of extra array creation. And you'll sometimes see Mergesort performing poorly because of that bug. – HARSHIT BAJPAI Jun 06 '19 at 16:23
  • @rcgldr I am currently doing this course in algorithms - https://www.coursera.org/learn/algorithms-part1/lecture/ARWDq/mergesort . If you go to week3's 3rd video and jump to 10 min, instructor is saying that it's important to not create the auxiliary array in the recursive routine because that could lead to extensive cost of extra array creation. And you'll sometimes see Mergesort performing poorly because of that bug. So all I want to do is write an implementation that does not create auxiliary array in recursion. That different from Bottom up approach. I am sorry, I mentioned wrong link – HARSHIT BAJPAI Jun 06 '19 at 16:31
  • @alexanderhurst I just updated the question. Can you please have a look once more. Thanks for the help so far. – HARSHIT BAJPAI Jun 06 '19 at 16:37
  • @rcgldr , I just updated the question. Can you please have a look at the question once more. Thanks for the help so far – HARSHIT BAJPAI Jun 06 '19 at 16:37
  • @HARSHITBAJPAI - I added an answer with fixes. – rcgldr Jun 06 '19 at 21:29

2 Answers2

1

I only have python 2.7. I didn't use a class. Fixes noted in comments.

def merge(array, aux_array, low, mid, high):
    for k in range(low, high+1):                # fix (high+1)
        aux_array[k] = array[k]                 # fix (array)
    i = low
    j = mid + 1
    for k in range(low, high+1):                # fix (high+1)
        if(i > mid):
            array[k] = aux_array[j]             # fix (j)
            j = j +1                            # fix (j)
        elif(j > high):
            array[k] = aux_array[i]             # fix (i)
            i = i +1                            # fix (i)
        elif(aux_array[i] <= aux_array[j]):     # fix (<=)
            array[k] = aux_array[i]
            i = i +1
        else:
            array[k] = aux_array[j]
            j = j +1

def mergesort(array, aux_array, low, high):     # fix (names)
    if(low >= high):                            # fix (size < 2)
        return                                  # fix (size < 2)
    mid = low + ((high - low)//2)               # fix (low +)
    mergesort(array, aux_array, low, mid)       # fix (names)
    mergesort(array, aux_array, mid+1, high)    # fix (names)
    merge(array, aux_array, low, mid, high)     # fix (names)

def start_sort(array):                          # fix (names)
    aux_array = [0] * len(array)                # fix allocate aux_array
    mergesort(array, aux_array, 0, len(array)-1)

arr = [10,2,30,0,4]

start_sort(arr)                                 # fix
print(arr)                                      # fix
rcgldr
  • 27,407
  • 3
  • 36
  • 61
  • The merge() function from corsera and in the example above has some inefficiencies, such as 3 conditionals for every element merged. This could be reduced to 2 conditionals (compare, check for end of run) per element merged. It's also possible to eliminate the copy in merge by changing the direction of merge based on level of recursion (or based on pass if doing bottom up merge sort). – rcgldr Jun 07 '19 at 01:35
0

Check in the merge sort method where you call mergesort(...) you should be calling self.mergesort(...)

This will solve the name not defined error and will get you a recursion depth error instead as you are recursively calling mergesort with no exit condition (though I assume this step was up next)

Good luck with the rest of the sort :)

edits I made for reference

    def mergesort(self, data, low, high):
        #high = len(data)
        mid = (high - low)//2
        self.mergesort(data, low, mid)      # <- 
        self.mergesort(data, mid+1, high)   # <-
        merge(data, aux_array, low, mid, high) 
alexanderhurst
  • 456
  • 3
  • 8
  • I did that. It says self is not defined – HARSHIT BAJPAI Jun 05 '19 at 18:03
  • Did you add self back into the arguments? edit since enter posts and doesnt add newline :S You had `def mergesort(self, data, low, high):` in the original method but `def mergesort(cls, data, low, high):` when you changed it to a class method – alexanderhurst Jun 05 '19 at 18:11
  • @HARSHITBAJPAI I added that function to my answer for better formatting – alexanderhurst Jun 05 '19 at 18:22
  • I actually did that as well. It says "NameError: name 'self' is not defined". If I replace self with merge_sort (i.e. class name), that error is gone. Also, self (or merge_sort) is also required with merge(...), i.e. in the last line of mergesort function. But after doing this, it gives error that aux_array is not defined in this location merge_sort.merge(data, aux_array, low, mid, high). So I replaced aux_array with merge_sort.aux_array. It works fine for this part. But then it throws an error at line 19 (in merge aux_array[k] = data[k] NameError: name 'data' is not defined ) – HARSHIT BAJPAI Jun 05 '19 at 20:22
  • I am sorry for troubling you. But if you can check and try to run on your own system here is my code - https://pastebin.com/ewYkSnCr – HARSHIT BAJPAI Jun 05 '19 at 20:24
  • And I did added the exit condition for WHILE loop – HARSHIT BAJPAI Jun 05 '19 at 20:24
  • Your object oriented structure has gone a little haywire. If you want to make this object oriented you have to revert mergesort back from a class method to what you had before and you have to do the same thing to merge. Or if you would prefer to use class methods you have to define what data is in your merge method and pass it into the merge operation. I recommend you take your original code and try just replacing the two lines in mergesort method as I have above – alexanderhurst Jun 06 '19 at 13:25
  • I actually tried that. If I run the code that you gave the link for, it is giving an error -merge(data, aux_array, low, mid, high) NameError: name 'merge' is not defined – HARSHIT BAJPAI Jun 06 '19 at 16:11
  • @HARSHITBAJPAI This issue is the same as the one that caused mergesort to be undefined. Merge should be an object function but you are referring to it like a class function. I converted merge to use the objects aux_array variable rather than an aux_array passed into the function. I also initialize aux_array in `__init__`. Code for that with comments on what changed can be found here https://pastebin.com/vE4jJ0xZ – alexanderhurst Jun 06 '19 at 17:44
  • I have not done anything about recursion depth. Just converted your functions to use their objects. Also since you don't appear to actually use the variable "data" passed into merge and mergesort it can be removed which will result in -> https://pastebin.com/bCm16rTR – alexanderhurst Jun 06 '19 at 17:50
  • Thank you so much for the corrections. I was checking out about the recursion depth. I have not been able to resolve it. This is the change I made - https://pastebin.com/BEzNC4b8 . Let me know if you have any ideas for this. If I am not able to resolve this, I might post a separate question for this – HARSHIT BAJPAI Jun 06 '19 at 18:34
  • I took a look at the video that you referenced. Your code doesn't line up with what they have for their merge function at 7 minutes. You should take a look to confirm that this is what you want. Take a close look at how your is and js compare with theirs. As for the original question, if it is solved please mark it as so and then you can make a new question about how to fix your implementation of mergesort – alexanderhurst Jun 06 '19 at 20:16