Skip to content

Conversation

@weijianzhg
Copy link
Contributor

For DisjointSets, it is useful if union! can return the root of the newly merged set.
For example, it is used in implementing elimination tree for sparse factorization.
(See Algorithm 5.1 of [2])

The implementation of _union! is based on [1].
After this merge, we have

julia> a = IntDisjointSets(10)
DataStructures.IntDisjointSets([1,2,3,4,5,6,7,8,9,10],[0,0,0,0,0,0,0,0,0,0],10)

julia> union!(a, 3, 5)
3

julia> a
DataStructures.IntDisjointSets([1,2,3,4,3,6,7,8,9,10],[0,0,1,0,0,0,0,0,0,0],9)

References:
[1] Data Structures and Network Algorithms, Robert Tarjan (1983) Chapter 2
Disjoint Sets [url]
[2] The Role of Elimination Trees in Sparse Factorization, Joseph W.H. Liu [url]

@weijianzhg
Copy link
Contributor Author

cc @jiahao @kmsquire

# merge the subset containing x and that containing y into one
#
function union!(s::IntDisjointSets, x::Integer, y::Integer)
parents = s.parents
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not used below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply. I think parents is used in find_root_impl! below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, sorry, I don't know how I missed that.

@kmsquire
Copy link
Member

kmsquire commented Feb 7, 2016

I think that makes sense and is a reasonable change. This can be merged after addressing the inline comments.

@kmsquire
Copy link
Member

kmsquire commented Feb 7, 2016

A couple of more (quick) requests: Please

  1. add a couple of quick tests that verify this functionality
  2. add an example to the documentation.
  3. squash your commits when ready to merge

@weijianzhg
Copy link
Contributor Author

@kmsquire all the requests are addressed. Please let me know if you have any other comments.

README.rst Outdated
Note that the internal implementation of ``IntDisjointSets`` is based on vectors, and is very efficient. ``DisjointSets{T}`` is a wrapper of ``IntDisjointSets``, which uses a dictionary to map input elements to an internal index.
Note that the internal implementation of ``IntDisjointSets`` is based on vectors, and is very efficient. ``DisjointSets{T}`` is a wrapper of ``IntDisjointSets``, which uses a dictionary to map input elements to an internal index.
As a result, ``union!(a, "a", "b")`` and ``root_union!(a, "a", "d")`` returns the indices of the
root elements rather than the root elements themselves.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering...

  1. Is there a way to go from index to element?
  2. If someone is manipulating non-integer DisjointSets, is returning the resulting index is useful? Can they use that index in further processing? How?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to go from index to element?

Since DisjointSets use a dictionary to wrap IntDisjointSets, "to go from index to element" means get a key by value in a dictionary. This is doable but not very efficient, perhaps something like

for (key, value) in intmap
     if value == index
          return key
     end
end

We may add a new field to DisjointSets, but this is a major change and shoud be considered in a different pull request. By the way, currently find_root only returns the index of the root rather than the root itself.

If someone is manipulating non-integer DisjointSets, is returning the resulting index is useful? Can they use that index in further processing? How?

Yes, the index can be used in further processing. Just use the simple example from the documentation.

> list = ["a", "b", "c", "d"]
> a = DisjointSets{AbstractString}(list)
> indx = union!(a, "a", "b")
> list[indx]
"a"

@kmsquire
Copy link
Member

@weijianzhang, sorry, I was about to merge, but it looks like something else snuck in that caused a merge conflict (probably in the exports). Can you rebase on the current master? After that, I thing this is mergable.

@weijianzhg
Copy link
Contributor Author

@kmsquire sure, no problem. I will fix the conflicts. Thanks.
Edit: I modified the README but it is now moved to Read the Docs, which causes the conflicts. It is now fixed.

@kmsquire
Copy link
Member

(Thanks... and can you squash those please?)

@weijianzhg
Copy link
Contributor Author

Hi @kmsquire, sorry but I don't know how. I can only squash consecutive commits but there are a few commits between my last commit and this commit. I've made a few attempts but was unsuccessful. Could you let me know how I can do this?

@kmsquire
Copy link
Member

Hi @weijianzhang, if you run

git fetch
git rebase origin/master

it should move your commits to the most recent position. From there, you should be able to squash and force push to this branch.

If that doesn't work for some reason, just post back here and we'll figure something out.

@DanielArndt
Copy link
Collaborator

@weijianzhang Looks like you merged the current master, instead of rebasing. Rebasing re-writes history as if your changes came after. Here's one way to fix:

git checkout weijianzhang/master
git rebase origin/master

You will need to fix the README conflicts here (simple, remove all the stuff on the bottom). I wouldn't bother adding the doc changes, since we can retrieve those from the merge you already did. When you're done

git rebase --continue

Take a look at what you're missing (from the merge you already did)

git diff ..weijianzhang/master

Apply those changes

git diff ..weijianzhang/master | git apply

Add your missing changes:

git add doc/source/disjoint_sets.rst

Amend them (add them) to your last commit (which should now, be your first commit)

git commit --amend

If everything looks right now, you should be able to push this to your remote.

@weijianzhg
Copy link
Contributor Author

@kmsquire @DanielArndt Thanks a lot for your help. I think it is fixed now.

@DanielArndt
Copy link
Collaborator

Great! The history looks a lot cleaner now.

kmsquire added a commit that referenced this pull request Feb 10, 2016
For DisjointSets, make union! return the root of the newly merged set
@kmsquire kmsquire merged commit 021d8a2 into JuliaCollections:master Feb 10, 2016
@rawls238
Copy link
Contributor

@kmsquire should we bump the version to v0.4.3 now that these recent changes have been merged in?

@kmsquire
Copy link
Member

Yep. Can you?

@rawls238
Copy link
Contributor

Sure, will do so tonight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants