Reputation: 896
I have a problem with (not anymore with stackoverflow (hehe)) Find algorithm when trying to implement UnionFind structure algorithm with path-compression.
I have standard array of ints, array can get pretty big -> it works fine until 60.000.000 elements.
My Union function looks like this:
public void unite(int p, int q) {
if(p >= 0 && p < id.length && q >= 0 && q < id.length){
if (isInSameSet(p, q)) return;
id[find(p)] = find(q);
stevilo--;
}
}
My isInSameSet looks like this:
public boolean isInSameSet(int p, int q) {
if(p >= 0 && p < id.length && q >= 0 && q < id.length)
return find(p) == find(q);
return false;
}
I have tried iterative way in Find:
public int find(int i) {
while (i != id[i]){
id[i] = id[id[i]];
i = id[i];
}
return i;
}
and tail-recrusion:
public int find(int i) {
int p = id[i];
if (i == p) {
return i;
}
return id[i] = find(p);
}
Is there anything I missed in my code? Is there any other approach to this kind of problems?
@edit: Adding constructor to code:
public UnionFind(int N) {
stevilo = N;
id = new int[N];
for(int i = 0; i < N; i++){
id[i] = i;
}
@edit2 (better explanation and new findings): The problem is not in stackoverflow anymore for less then 60.000.000 elements, which is more then enough for solving my problems.
I'm calling test Unions like this:
for(i=0;i<id.length-1;i++)
unite(i,i+1)
so the ending pairs are like this:
0:1, 1:2, 2:3, 3:4,..
which is only example of least optimal option for testing means only :)
Then I check if representative of 0 is last element in table (99 for 100 elements) and it works.
Problem is, that my algorithm works only if initial elements are each in their own union (0:0, 1:1, 2:2, 3:3). If I have different Unions already set up (0:2, 1:6, 2:1, 3:5, ...) my testing algorithm stops working.
I have narrow it down to a problem in Find function, probably something to do with path compression
id[i] = id[id[i]].
Upvotes: 4
Views: 1593
Reputation: 3023
I once wrote an algorithm for UnionFind
, and its time complexity is O(log*(n)). Thats iterative logarithm of n. The algorithm compresses the path of the tree as it keeps on connecting the nodes to gain efficiency. I find it very efficient, though I haven't practically tested it against huge array size. Here's the code:
public class UnionFind
{
private int[] id;
public UnionFind(int capacity)
{
id = new int[capacity];
for (int i = 0; i < capacity; i++)
{
id[i] = i;
}
}
public boolean isConnected(int p, int q)
{
return root(p) == root(q);
}
public void connect(int p, int q)
{
if (isConnected(p, q))
{
return;
}
id[root(p)] = root(q);
}
private int root(int p)
{
int temp = p;
if (p != id[p] && id[id[p]] != id[p])
{
while (p != id[p])
{
p = id[p];
}
id[temp] = id[p];
}
return id[p];
}
}
Upvotes: 1
Reputation: 4955
Union-Find data structures typically include TWO different optimizations. One is path compression. You have that.
But the other optimization happens during a Union, where you carefully choose which of the two roots to make a child of the other, usually via Union-By-Rank or Union-By-Size. With that optimization, your trees should never be deep enough to get a stack overflow. However, that optimization seems to be missing from your unite function.
Upvotes: 2
Reputation: 12563
One small optimization would be to get rid of isInSameSet...
public void unite(int p, int q) {
if(p >= 0 && p < id.length && q >= 0 && q < id.length){
int rootp = find(p);
int rootq = find(q);
if (rootp==rootq) return;
id[rootp] = rootq;
stevilo--;
}
}
Upvotes: 2