views:

71

answers:

2

I've written a custom comparator to compare my node classes, but the java priority queue is not returning my items in the correct order.

Here is my comparator:

public int compare(Node n1, Node n2){

    if (n1.getF() > n2.getF()){
        return +1;
    }
    else if (n1.getF() < n2.getF()){
        return -1;
    }
    else {  // equal
        return 0;
    }
}

Where getF returns a double. However after inserting several Nodes into the priority queue, I print them out using:

while(open.size() > 0) {
    Node t = (Node)(open.remove());
    System.out.println(t.getF());
}

Which results in:

6.830951894845301
6.830951894845301
6.0
6.0
5.242640687119285
7.4031242374328485
7.4031242374328485
8.071067811865476

Any ideas why this is so? Is my comparator wrong? Thanks.

Mike

+4  A: 

How are you printing out those values? I don't think the iterator from PriorityQueue provides the same ordering assurances that the overall class does, so potentially if you're doing

for(Node n : queue) {
System.out.println(n.getF());
}

You'll be getting unordered output. The ordering assurance only applies to offer, take, poll, peek, and possibly some other methods.

There's a special mention on the iterator in the javadocs for priority queue http://java.sun.com/javase/6/docs/api/java/util/PriorityQueue.html

Ceilingfish
That's correct; from the `PriorityQueue` javadoc for the `iterator()` method: "The iterator does not return the elements in any particular order."
Richard Fearn
"How are you printing out those values?".... The question says "I print them out using...".
aioobe
I am not using the iterator.Comparator<Node> comparator = new NodeComparator();PriorityQueue<Node> open = new PriorityQueue<Node>(INITIAL_SIZE, comparator);And then the above while loop.
Michael Simpson
+1  A: 

Don't know what's wrong with your code, but this works for me:

import java.util.*;
public class Test {
    public static void main(String[] args) {
        PriorityQueue<Node> open = new PriorityQueue<Node>(10,
                new Comparator<Node>() {
            @Override
            public int compare(Node n1, Node n2){
                if (n1.getF() > n2.getF()){
                    return +1;
                }
                else if (n1.getF() < n2.getF()){
                    return -1;
                }
                else {  // equal
                    return 0;
                }
            }
        });

        for (int i = 0; i < 20; i++)
            open.add(new Node());

        while(open.size() > 0) {
            Node t = (Node)(open.remove());
            System.out.println(t.getF());
        }
    }
}

class Node {
    double d = Math.random() * 10;
    public double getF() { return d; }
}

Output:

0.21442281608773262
1.9965384843480016
2.6660026888929824
2.888889937975976
3.098932914222398
3.1059072964534638
4.193212975907516
4.296282412431935
4.3241392173963735
4.825876226139123
5.193550353435191
5.637831708672641
5.949759449054407
6.620639629878806
7.505126870725806
7.966337123623846
8.270840212631589
8.484502118941545
8.730910327480023
9.191324325662219

Make sure getF() doesn't accidentally return an int-version of the double.


Update: You cannot update the data which defines the order of the elements after insertion. In that case you need to extract the element, updated it, and reinsert it.

aioobe
Can you try using the class iterator? See if you get unordered output?
Ceilingfish
Then it's not ordered correctly. Makes sense. If it's implemented as a heap, all it knows is which the next element to remove is.
aioobe
Hmm... well I add the nodes to the prio queue first, then use the Node's "setF" function to change the f values after. Is there something about values not being updated in the prio queue after they object has been inserted? Even though I am pointing to the node in the prio queue?@Ceilingfish - my node class initializes the f value to -1 and then inserts into prio queue then updates f val. Which is different than your above example. I appreciate you double checking the basic logic though, thanks.
Michael Simpson
PriorityQueue inserts the data in the correct location. The data is not resorted before it is it is polled. If you are changing the data after insertion it will not be ordered.
Jacob Tomaw
Thanks Jacob, is there a standard way to get around this?
Michael Simpson
Jacob, you should probably turn this comment into an answer so you can get credit for it (and so that people looking at the question know where to find the answer of course)
DJClayworth
"is there a standard way to get around this?"Yes, don't change data after it's added to an ordered collection. It's usually best practice to only use immutable objects in ordered collections if you want them to stay ordered.
Skip Head
I'm implementing the A* search algorithm: http://en.wikipedia.org/wiki/A*_search_algorithm. Following the pseudo code, the open set is a prio queue. Except calculating the appropriate f values happens after inserting the nodes into "open". Thus I am inserting nodes into "open" then calculating their f values. Any ideas on how to work around this? My procedure is what I figured would be the equivalent java implementation, but clearly I have hit a snag here. The pseudo code syntax suggests array's holding all the g, h, and f values, but I have it so that each node keeps track of its own values.
Michael Simpson