ImmutableComputableGraph code improvement + unit tests - #1042
Conversation
mbabadi
commented
May 16, 2017
mbabadi
commented
- reviewed and restricted the access modifiers of all ICG-related classes
- got rid of the functionality to hold on to old caches: whenever a cache goes out of date, the reference is immediately made null in the new ICG
- completely rewrote ComputableGraphStructure in a functional style
- got rid of unused and unnecessary methods
- Created an ImmutableComputableGraphUtils and factored out the builder and other common methods
- math equality asserts for NDArray
- unit tests for ComputableGraphStructure
- unit tests for ImmutableComputableGraph
- unit tests for ImmutableComputableGraphUtils
|
@samuelklee can you suggest a reviewer? this is smaller than it seems :) |
|
@asmirnov239, perhaps you can take a look after you issue the PR for the germline WDL? Otherwise I will take it when I get back, if you don't get to it by then. |
|
Yes sure! I can review it after I come back Wednesday next week |
asmirnov239
left a comment
asmirnov239
left a comment
There was a problem hiding this comment.
@mbabadi Looks great! Back to you
| */ | ||
| public ComputableCacheNode duplicateWithUpdatedValue(final Duplicable newValue) { | ||
| if (cacheEvals && newValue != null && !newValue.hasValue()) { | ||
| ComputableCacheNode duplicateWithUpdatedValue(final Duplicable newValue) { |
There was a problem hiding this comment.
missing @Override annotation
| * - topological order for evaluating all nodes associated to a tag (see {@link ImmutableComputableGraph}). | ||
| * - assertion for existence of no cycles | ||
| * - construction of maps of nodes to their descendants and ancestors | ||
| * - propagation of tags from descendants to ancestors |
There was a problem hiding this comment.
Can you add a description, or a link to a description of what tag is
| * - topological order for evaluating a computable node | ||
| * - topological order for mutating a primitive/externally-computed node and updating the caches of all involved nodes | ||
| * - topological order for evaluating all nodes associated to a tag (see {@link ImmutableComputableGraph}) | ||
| * - topological order for complete computation of the graph |
There was a problem hiding this comment.
topological order for complete computation of the graph -> topological order for evaluating all nodes in the graph
| nodeTagsSet = extractTags(nodeSet); | ||
| assertParentKeysExist(nodeSet, nodeKeysSet); | ||
|
|
||
| /* nodeKey -> set of parents' keys */ |
There was a problem hiding this comment.
I would move this description to the instance variable definition or the getter methods
| /* nodeKey -> set of parents' keys */ | ||
| parentsMap = getParentsMap(nodeSet); | ||
|
|
||
| /* nodeKey -> set of children's keys */ |
There was a problem hiding this comment.
Same goes here, and for everything else below
| private static final Map<String, Duplicable> EMPTY_PARENTS = new HashMap<>(); | ||
|
|
||
| /** | ||
| * Instructions for computing f(x, y) = F[2] ( F[0](x), F[1](x) ) |
There was a problem hiding this comment.
I think it should be F[2] ( F[0](x), F[1](y) )
| Assert.assertTrue(!icg_tmp.isValueDirectlyAvailable("h")); | ||
| assertIntactReferences(icg_0, icg_tmp, "y", "z", "g"); | ||
|
|
||
| ImmutableComputableGraph icg_tmp_old = icg_tmp; |
There was a problem hiding this comment.
Why declare another reference?
| * Tests propagation of tags from descendents to parents | ||
| */ | ||
| @Test(dataProvider = "allPossibleNodeFlags", invocationCount = NUM_TRIALS) | ||
| public void testTagPropagation(final boolean f_caching, final boolean f_external, |
There was a problem hiding this comment.
Shouldn't this belong to ComputableGraphStructureUnitTest?
There was a problem hiding this comment.
This test is actually redundant given the tests in ComputableGraphStructureUnitTest but I thought of keeping it because it is a concrete test for the concrete ICG worked out in this unit test. It may help some person in the future to understand what tag propagation means, etc.
| * Asserts that the equality comparison of two {@link CacheNode}s is done just based on their key | ||
| */ | ||
| @Test | ||
| public void testEquality() { |
There was a problem hiding this comment.
This test should go into the corresponding CacheNode unit test
| @Test(enabled = false) | ||
| public void testPrimitiveUpdating() { | ||
| throw new AssertionFailedError("Test is not implemented yet"); | ||
| @Test(expectedExceptions = UnsupportedOperationException.class) |
There was a problem hiding this comment.
Same here as above, and also for the tests below
|
@mbabadi Migration instructions for this branch: https://github.com/broadinstitute/gatk/wiki/Migrating-branches-from-gatk-protected-to-gatk |
- reviewed and restricted the access modifiers of all ICG-related classes - got rid of the functionality to hold on to old caches: whenever a cache goes out of date, the reference is immediately made null in the new ICG - completely rewrote ComputableGraphStructure in a functional style - got rid of unused and unnecessary methods - Created an ImmutableComputableGraphUtils and factored out the builder and other common methods - math equality asserts for NDArray - unit tests for ComputableGraphStructure - unit tests for ImmutableComputableGraph - unit tests for ImmutableComputableGraphUtils - keys and tags have their own classes now
|
@asmirnov239 OK, done with the review! :) I addressed all of your comments except for one (concrete example for graph structure). Will move to main repo and merge after the tests pass. |
d6a5bdf to
8282bd4
Compare
4 participants
