49 add union find data structure - #62
Conversation
…tor implementation
…concerns and notes in a comment
…t for subsets to HashMap
… mismatches that will be sorted out later on if I decide to stick with HashMaps
…by size in UnsortedUnionFind
…w private method contains(T e)
…ure maps fulfil set requirement
…) as static method problematic due to variable type
…ionFind; adapt interfaces accordingly
…by size in SortedUnionFindTest
…rtedness will be tested separately)
…o ParentPointerTreeUnionFind
This reverts commit 36d5aaa.
… union methods slightly
There was a problem hiding this comment.
The implementation currently lacks documentation in many places and has outdated/wrong documentation in other places.
Class structure needs to be improved urgently, as it blocks several default methods/concepts (e.g. Builder) from being implemented correctly, and also mixes multiple distinct implementations. In other cases, e.g. find(), there is no need for having multiple distinct implementations, yet there are multiple 1:1 duplicates.
Several immutable or persistent classes have SuppressWarnings("Immutable") annotations that hide real problems. The biggest of which is that the ImmutableUnionFind is not immutable, since the tree is not immutable. Modifying nodes modifies other copies of the same tree. This is also not covered by tests.
Furthermore, there are code duplicates in multiple places that need to be reduced (I did not mark all, but they should be obvious when working on the marked cases).
While tests cover default behavior, they miss type specifics quite often. But they too suffer from a structural problem, in that there is lots of duplicate code or test cases (ParentPointerTreeUnionFindByRankTest and ParentPointerTreeUnionFindBySizeTest are more or less direct duplicates), type inheritance is not used to test multiple union-finds at the same time etc. This would help with all test problems!
Please refactor the tests into abstract, generic, parameterized tests.
Also, don't modify test class variables in multiple places before using them (e.g. SortedUnionFindTest), as it becomes really hard to follow this, leading to tests easily being based on wrong assumptions.
Several tests for sorted variants test unsorted variants (e.g. PersistentParentPointerTreeUnionFind is tested instead of PersistentSortedParentPointerTreeUnionFind).
The tests for immutable versions don't test mutations of shared existing nodes (see above).
Tests are more or often then not undocumented. This is no problem for trivial tests, but more complex tests need to be documented.
Optimizations are effectively untested.
| import org.sosy_lab.common.collect.union_find.ImmutableSortedParentPointerTreeUnionFind; | ||
| import org.sosy_lab.common.collect.union_find.ParentPointerTreeUnionFind.UnionType; | ||
|
|
||
| public class ImmutableParentPointerTreeUnionFindSortednessTest { |
There was a problem hiding this comment.
The name can be simplified:
| public class ImmutableParentPointerTreeUnionFindSortednessTest { | |
| public class ImmutableSortedUnionFindTest { |
Possibly even more, depending on whether you test its immutability in this class or not (which would be totally legit).
Specify type information should only be included if you actually test its specifics.
As far as i can see, this only tests sortedness, hence neither immutability nor the implementation specifics of the UnionFind (i.e. that it is a ParentPointerTree or Tree) matter here.
| } | ||
|
|
||
| @Test | ||
| public void testGetAllSubsets_elementsAddedInAscendingOrder_remainSorted() { |
There was a problem hiding this comment.
Our Java naming convention (see Google style guide) uses camel case, not snake case.
Also, tests should add the suffix *Test always at the end of their name.
This should be considered in the entire MR.
| // tree nodes are not immutable but only used internally and never mutated after creation | ||
| // immutable tree nodes would make conversion during build() difficult and time-consuming | ||
| @SuppressWarnings("Immutable") | ||
| private final ImmutableMap<T, AbstractTreeNode<T>> allNodes; |
There was a problem hiding this comment.
This is not a immutable UnionFind implementation. While your Map is immutable, the nodes, and therefore the tree, are not immutable. Modifying a previous copy also modifies this copy and vice versa.
| @Override | ||
| public Collection<? extends Set<T>> getAllSubsets() { | ||
|
|
||
| Map<T, Set<T>> allSubsets = new HashMap<>(); |
There was a problem hiding this comment.
This is a immutable class. Why not return a immutable collection here?
| super(pUnionType); | ||
| } | ||
|
|
||
| public static <T> AbstractImmutableParentPointerTreeBuilder<T> getBuilder( |
There was a problem hiding this comment.
The static constructor method for Builder should be part of the public interface of the immutable classes, and this method should be moved there.
The idea of a Builder is that you automatically get the correct class when requesting it through the static constructor method. But here you hard-coded them, making this impossible. A user needs to know the details of the class at all points when programming, making this inconvenient.
|
|
||
| Preconditions.checkNotNull(pE); | ||
|
|
||
| List<AbstractTreeNode<T>> toBeCompressed = new ArrayList<>(); |
| } | ||
|
|
||
| @CanIgnoreReturnValue | ||
| public AbstractImmutableParentPointerTreeBuilder<T> add(Set<T> pSet) { |
There was a problem hiding this comment.
Missing documentation.
What happens if this set contains elements that are already in the union-find?
| * @param <T> The type of values. | ||
| */ | ||
| @Immutable(containerOf = "T") | ||
| public interface PersistentSortedUnionFind<T extends Comparable<T>> extends SortedUnionFind<T> { |
There was a problem hiding this comment.
This should be a subtype of PersistentUnionFind.
Also, the type bounds are restrictive and disallow naturally ordered elements if the ordering is defined in a superclass. Please change this to T extends Comparable<? super T>.
|
|
||
| public class ImmutableParentPointerTreeUnionFindTest { | ||
|
|
||
| private final int[] simpleUnionArgs = new int[] {0, 1}; |
There was a problem hiding this comment.
Why not just use the values? Why does this exist at all?
| // union-find not immutable but only used internally and never mutated passed outward | ||
| // build() returns an immutable union-find that contains a copy of this union-find's map | ||
| @SuppressWarnings("Immutable") | ||
| final ParentPointerTreeUnionFind<T> unionFind; |
baierd
left a comment
baierd
left a comment
There was a problem hiding this comment.
Furthermore, the node implementations do not define equals() and hashcode(). This is not wrong, since nodes are not re-introduced in the implementations. But there are two problems due to this:
- the code is fragile, since expression like this
while (!node.equals(parent))depend on reference equality implicitly now. Making this explicit helps code readability and future developers and maintainers. Switching to explicit reference equality should be considered. - sorted implementations have a equals/compareTo mismatch. Example:
new BigInteger("1.0").equals(new BigInteger("1.00")) // false
new BigInteger("1.0").compareTo(new BigInteger("1.00")) // 0, i.e. equal
I am currently unsure whether this can lead to problems in the current implementation.
Defining equals() and hashcode(), avoids these problems.
2 participants
So far, this includes the interfaces UnionFind, SortedUnionFind, PersistentUnionFind and PersistentSortedUnionFind as well as the abstract classes AbstractImmutableUnionFind and AbstractImmutableSortedUnionFind. It also contains a naive implementation of SortedUnionFind based on a HashMap of TreeSets (SortedTreeSetUnionFind). There is a set of tests (SortedUnionFindTest), but there seems to be some kind of problem regarding null values which is causing the JDK21 PackageSanityTest to fail. This in particular is something I'd appreciate input on. Thanks!