Remove compact node manager #122
Conversation
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
Java/core/src/main/java/com/amazon/randomcutforest/tree/AbstractCompactRandomCutTree.java
Outdated
Show resolved
Hide resolved
In a previous revision, we introducted the CompactNodeManager class to unify the handling of leaf nodes and internal nodes. The initial implementation was a leaky abstraction, because the tree class was still responsible for managing some node properties, like bounding boxes. The change moves node management back into the tree class.
jotok
force-pushed
the
remove-node-manager
branch
from
cf7c9f8
to
87927b4
Compare
| } | ||
|
|
||
| @Override | ||
| public void setParent(int index, int parent) { | ||
| parentIndex[index - super.capacity] = (short) parent; | ||
| parentIndex[index] = (short) parent; |
There was a problem hiding this comment.
question. we plan to eliminate unsafe type conversions in the code? if so, there is an issue?
There was a problem hiding this comment.
I think we will have a "large" node store/leafstore if the sampleSize is large (for large trees). Otherwise we have these small trees (but a large number of them). The trees always see int (irrespective of large/small size). The ILeafStore/INodeStore interfaces are int.
|
|
||
| public AbstractCompactRandomCutTree(int maxSize, long seed, boolean enableCache, boolean enableCenterOfMass, | ||
| boolean enableSequenceIndices) { | ||
| super(seed, enableCache, enableCenterOfMass, enableSequenceIndices); | ||
| checkArgument(maxSize > 0, "maxSize must be greater than 0"); | ||
| this.maxSize = maxSize; | ||
| nodeManager = new CompactNodeManager(maxSize); | ||
| leafStore = new LeafStore((short) maxSize); |
There was a problem hiding this comment.
This can be an int (used just once in a constructor).
| return getPointReference(intValue(node)); | ||
| } | ||
|
|
||
| int getPointReference(int index) { |
There was a problem hiding this comment.
I think the access level in this code base can be more consistent. For example,
- The interface of an object is public.
- Everything else should be private by default.
- If needed, protected is better than package since there is no reason to deny subclasses what is not private.
| } | ||
|
|
||
| protected int incrementMass(int node) { | ||
| return isLeaf(node) ? leafStore.incrementMass(getLeafIndexForTreeIndex(node)) : nodeStore.incrementMass(node); | ||
| } | ||
|
|
||
| @Override | ||
| protected Integer getSibling(Integer node) { |
There was a problem hiding this comment.
minor. maybe the return type of the interface should be primitive since it's not nullable.
There was a problem hiding this comment.
getSibling is not nullable -- but the AbstractCompactTree can use either reference to Node or Integer ... so the non-primitive type came from the generics in AbstractCompactTree
Remove the
CompactNodeManagerclass and move the functionality to `AbstractCompactRandomCutTree.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.