7

I'm working on a program which sorts an array by dividing it into smaller max-heaps and extracting the max-integer out of each one, then deleting it from the heap and running again until every heap is empty, but I can't seem to figure it out.

From where I stand the code looks good, but I don't get the results which I am looking for. My input is created randomly, and makes an array of 512 integers. Here is what it prints for one example run -

Original Array  -391 176 -380 -262 -474 327 -496 214 475 -255 50 -351 179 -385 -442 -227 465 127 -293 288
Sorted Array 475 465 327 327 327 327 327 327 327 327 327 327 327 327 327 327 327 327 327 327 
n = 20 k = 2
The number of comparisons is 243

Can anyone spot what's wrong with my code? I will be really gladful.

(1) Main Program

 import java.io.File;
 import java.util.*;
 import java.io.FileNotFoundException;
 import java.util.Scanner;
 import java.io.IOException;

 public class Project {
    static final int n = 20;
    static final int k = 2;
    static int counter = 0;
    private static Scanner scan;

    public static void main(String[] args) throws IOException {
        // Optional  - reading from a file containing 512 integers.
        InputCreator.main();
        File f = new File("random.txt");
        // File f = new File("increase.txt");
        // File f = new File("decrease.txt");
        try { scan = new Scanner(f);
        } catch (FileNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace(); }
        int [] L = new int[n];
        System.out.print("Original Array ");
        for (int i = 0; i < n ; i++)
            { counter++; L[i] = scan.nextInt(); System.out.print(" " + L[i]); }
        Projectsort(L);
    }

    private static void Projectsort(int [] L) {
        // int [][] Li = new int [k] [n-(n/k*(k-1))]; // The size of the rest of the integers (n-(n/k*(k-1)) will always be bigger than n/k
        int [] temp = new int [n/k], extra = new int [n-(n/k)*(k-1)];
        int extraIndex = 0, max, maxIndex = 0, r = 0;
        ProjectMaxHeap [] Li = new ProjectMaxHeap [k];
        // The following loop's time effiency is O(k) * O(N/k) = O(N)
        for (int i=0; i<k-1; i++) { counter++; // copying all the integers from Array L into K-1 smaller arrays
            for (int j=0; j<n/k ; j++)
                 { counter++; temp [j] = L[i*(n/k)+j]; }
            Li[i] = new ProjectMaxHeap (temp); }

        for (int i=(n/k)*(k-1) ; i<n ; ++i) // The rest of the integers on array L
            { counter++; extra [extraIndex] = L[i]; extraIndex++; }
        Li[k-1] = new ProjectMaxHeap(extra);
        System.out.print("\nSorted Array ");
        for (int i = n ; i > 0 ; i--) { counter++;
            r = 0;
            do{max = Li[r].extractMax(); r++; }while(Li[r].isEmpty() && r < k - 1);
            for (int j = r; j < k; j++) // Time efficiency O(k)*O(N/k)
            { counter++;
              if(!Li[j].isEmpty()) {
                if (Li[j].extractMax() > max) { 
                    counter++;
                    max = Li[j].extractMax(); 
                    maxIndex = j; } 
            } 
        System.out.print(max + " ");
        Li[maxIndex].deleteMax(); } }
        System.out.println("\nn = " + n + " k = " + k +"\nThe number of comparisons is " + counter);
    }
 }

(2) Max Heap Class

    public class ProjectMaxHeap
{
    private int [] _Heap;
    private int _size;

    public ProjectMaxHeap (int [] A){
        _size = A.length;
        _Heap = new int[A.length]; 
        System.arraycopy(A, 0, _Heap, 0, A.length);
        for (int i = _size / 2 ; i >=0 ; i--) {
            Project.counter++;
            maxHeapify(i); }
    }

    private int parent(int pos) 
    { return pos / 2; }

    private int leftChild(int pos)
    { return (2 * pos); }

    private int rightChild(int pos)
    { return (2 * pos) + 1; }

    private void swap(int fpos,int spos) {
        int tmp;
        tmp = _Heap[fpos];
        _Heap[fpos] = _Heap[spos];
        _Heap[spos] = tmp; }

    private void maxHeapify (int i) {
        int l = leftChild(i), r = rightChild(i), largest;
        if(l < _size && _Heap[l] > _Heap[i]) {
            Project.counter+=2;
            largest = l; }
            else largest = i;
        if(r < _size && _Heap[r] > _Heap[largest]) {
            largest = r;
            Project.counter+=2; }
        if (largest != i) {
            Project.counter++;
            swap(i, largest);
            maxHeapify (largest); }
        } 

    protected boolean isEmpty() { return _size == 0; }

    protected void deleteMax() {
        if (_size > 1) {
            Project.counter++;
            maxHeapify(0);
            int max = _Heap[0];
            _size--;
            swap(0, _size);
            maxHeapify(0); }
        else _size = 0;    
    }

    protected int extractMax() {
        maxHeapify(0);
        return _Heap[0];
    }
}

(3) Input Creator

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.util.*;
import java.io.FileReader;
import java.io.BufferedReader;

public class InputCreator {
    public static void main() {
        randomizer();
        decrease();
        increase();
    }
    private static void randomizer() {
        // The target file
        File out = new File("random.txt");
        FileWriter fw = null;
        int n = 0;
        // Try block: Most stream operations may throw IO exception
        try {
            // Create file writer object
            fw = new FileWriter(out);
            // Wrap thק writer with buffered streams
            BufferedWriter writer = new BufferedWriter(fw);
            int line;
            Random random = new Random();
            while (n < Project.n) {
                // Randomize an integer and write it to the output file
                line = random.nextInt(1000)-500;
                writer.write(line + "\r\n");
                n++;
            }
            // Close the stream
            writer.close();
        } catch (IOException e) {
            e.printStackTrace();
            System.exit(0);
        }
    }
    private static void increase() {
        // The target file
        File out = new File("increase.txt");
        FileWriter fw = null;
        int n = 0;
        int temp = 0;
        // Try block: Most stream operations may throw IO exception
        try {
            // Create file writer object
            fw = new FileWriter(out);
            // Wrap thק writer with buffered streams
            BufferedWriter writer = new BufferedWriter(fw);
            int line;
            Random random = new Random();
            while (n < Project.n) {
                // Randomize an integer and write it to the output file
                line = random.nextInt((n+1)*10);
                if(line > temp) {
                writer.write(line + "\r\n");
                n++; 
                temp = line; }
            }
            // Close the stream
            writer.close();
        } catch (IOException e) {
            e.printStackTrace();
            System.exit(0);
        }
    }
        private static void decrease() {
        // The target file
        File out = new File("decrease.txt");
        FileWriter fw = null;
        int n = 0;
        int temp = 10000;
        // Try block: Most stream operations may throw IO exception
        try {
            // Create file writer object
            fw = new FileWriter(out);
            // Wrap thק writer with buffered streams
            BufferedWriter writer = new BufferedWriter(fw);
            int line;
            Random random = new Random();
            while (n < Project.n) {
                // Randomize an integer and write it to the output file
                line = 10000 - random.nextInt((n+1)*20);
                if(line < temp) {
                writer.write(line + "\r\n");
                n++; 
                temp = line; }
            }
            // Close the stream
            writer.close();
        } catch (IOException e) {
            e.printStackTrace();
            System.exit(0);
        }
    }
}
Eddie Romanenco
  • 311
  • 4
  • 16
  • 3
    Why do you need multiple heaps? If you have smaller heaps I'd expect heapsort for each block first and than mergesort to combine results... In any case write unit test for your heap code and merging code separately so you can scope down your problem . – Alexei Levenkov May 20 '15 at 22:17
  • It's part of an assignment given to me, as funny as it seems - it has to work this way. First dividing the array into k heaps - then extracting the max of each one, deleting the largest max of its heap and repeating the process. – Eddie Romanenco May 20 '15 at 22:21
  • What is it that is being printed out? Apparently not the content of the sorted array, as even for an array of size 16 it prints many many numbers. Also, are you aware that you delete the contents of `temp` (in `Projectsort`) in each iteration of the loop? – Kulu Limpa May 20 '15 at 22:59
  • I should be printing the content of the sorted array (sorted from the largest int to the smallest). And the temp array's content is supposed to be temporary, after I create a heap from its content I don't care for it anymore. – Eddie Romanenco May 20 '15 at 23:06
  • Be careful though, in `ProjectMaxHeap`, you don't copy the contents of `A` (which is the array `temp`), but simply assign the same reference to `_Heap`. Once you rewrite the contents of `temp`, you rewrite the contents of `_Heap`, because they are two references to the same array object. Make sure to copy the array: `_Heap = new int[A.length]; System.arraycopy(A, 0, _Heap, 0, A.length);` – Kulu Limpa May 20 '15 at 23:11
  • Thanks! Anything else that might help, I feel like i'm missing something so basic that it hurts my brain. – Eddie Romanenco May 20 '15 at 23:25
  • You never use `parent`, so maybe you forgot something? Also `Li[maxIndex]._Heap = Li[maxIndex].deleteMax(Li[maxIndex]._Heap);` could be reduced to `Li[maxIndex].deleteMax()` if you change the method signature, since the method is already modifying the heap. Generally, don't pass the fields of `ProjectMaxHeap` to methods of `ProjectMaxHeap` - they are already accessible. I suggest making the fields private to restrict access from `Project`. Now, those are just refactoring suggestions that shouldn't change the behavior, but I guarantee you that debugging clean, i.e., readable, code is easier. – Kulu Limpa May 20 '15 at 23:50
  • I've made a few changes, still not working as I would like it to be. Am I starting to catch your drift? – Eddie Romanenco May 21 '15 at 00:15
  • We're getting there. If you had 20 reputation we could chat - oh well... `counter++` seems a bit randomly placed (not just at comparisons), but I'd ignore that for now. I think you should further inspect `heapify`, as there seem to be issues with that method. Make a new `main` method, create a heap containing about 8 elements, and debug `maxHeapify`. Also, consider setting `k = 1` to test whether your algorithm works if you don't split your data. – Kulu Limpa May 21 '15 at 00:51
  • OK, things are starting to look real nifty. When I don't make the division the array is being sorted out well. For example for - n = 20, k = 1 original array is `-385 -466 -321 -284 -476 -124 -251 493 -433 -265 29 25 -213 426 -285 343 -191 -204 -30 213` and the sorted would then be `493 426 343 213 29 25 -30 -124 -191 -204 -213 -251 -265 -284 -285 -321 -385 -433 -466 -476`. The division still doesn't work, say we will be getting an array of `-108 300 263 -331 64 477 -31 -447 417 98 116 -478 -320 251 241 292 -418 -328 333 54`. – Eddie Romanenco May 21 '15 at 05:40
  • When I change k to 2 - the sorted array I am getting is `477 477 300 333 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263 263`, and yes, I have noticed that the sorted array is way larger than the original. – Eddie Romanenco May 21 '15 at 05:40
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/78374/discussion-between-eddie-romanenco-and-kulu-limpa). – Eddie Romanenco May 21 '15 at 05:52
  • Alright, so now I narrowed the sorted array to match the size of the original one, but it still "locks" on one number until the end. Still, it looks like i'm close to the solution. – Eddie Romanenco May 21 '15 at 06:49

1 Answers1

2

The problem is with max = Li[0].extractMax(); You are not checking if Li[0] might be empty.

Always check preconditions and fail fast. The problem would have become immediately obvious had you started extractMax and deleteMax with

if (_size == 0) {
    throw new IllegalStateException("empty heap");
}

Here's the fixed final loop:

for (int i = 0; i < n; i++) {
    int maxIndex = -1;           // remove these variable declarations from top of method
    int max = Integer.MIN_VALUE; // it's best to confine variables to narrow scope
    for (int j = 0; j < k; j++) {
        if (!Li[j].isEmpty()) {
            int current = Li[j].extractMax();
            if (maxIndex == -1 || current > max) {
                maxIndex = j;
                max = current;
            }
        }
    }
    assert maxIndex != -1;
    Li[maxIndex].deleteMax();
    System.out.print(max + " ");
}
Community
  • 1
  • 1
Misha
  • 27,433
  • 6
  • 62
  • 78
  • Would this make it any better? `r = 0; do{max = Li[r].extractMax(); r++; }while(Li[r].isEmpty() && r < k - 1);` – Eddie Romanenco May 21 '15 at 06:21
  • Have you actually tried this new code? It looks wrong. Did you update your `extractMax` and `deleteMax` with error checking as I suggested? – Misha May 21 '15 at 06:46
  • 1
    @EddieRomanenco When you ask for help with finding a bug and get an answer, do not edit your question and remove the bug from it. It makes the answer not make any sense to anybody else. – Misha May 21 '15 at 06:54
  • Yes to both questions. Now the sorted array matches the size of the original one, but when I run it, it "locks" on one number until the end. For example: original = `-486 -457 -286 285 -22 -139 -380 25 163 -388 452 -31 461 -416 482 -491 193 -355 -464 -219`, sorted = `482 461 452 285 285 285 285 285 285 285 285 285 285 285 285 285 285 285 285 285 ` – Eddie Romanenco May 21 '15 at 06:55
  • If you added the error checking to `deleteMax`, your new code should be failing with an exception. You are not correctly setting `maxIndex` on every iteration. – Misha May 21 '15 at 06:58
  • Now I see it. I've corrected it to `r = 0; do{max = Li[r].extractMax(); maxIndex = r; r++; }while(Li[r].isEmpty() && r < k - 1);` I think it works now. – Eddie Romanenco May 21 '15 at 07:06
  • 1
    Well, you suffered enough. I updated the answer with a loop that should work correctly. – Misha May 21 '15 at 07:12
  • No, fails. Although it did work for one iteration for some weird reason. But now I try to extract numbers from an empty array and I see why. I'll try to toy around with my code. – Eddie Romanenco May 21 '15 at 07:14