Fix bug in truncated_graph_inputs #113
Conversation
| else: | ||
| # populate search otherwise | ||
| # it is a new independent node not present in ancestors to include | ||
| if dependent: |
There was a problem hiding this comment.
Changed the order of the ifelse, but not the logic
ricardoV94 force-pushed the fix_truncated_graph_inputs_bug branch from 5b60ca2 to f53e356 Compare tests/graph/test_basic.py Outdated
| trunc_inp2 = MyOp(x, y) | ||
| trunc_inp2.name = "trunc_inp2" | ||
| | ||
| o = MyOp(trunc_inp1, trunc_inp1, trunc_inp2, trunc_inp2) |
There was a problem hiding this comment.
There seems to be another problem with nested inputs:
The following asserts are what the function returns. Which one is correct, and why is it order dependent?
def test_truncated_graph_inputs_repeated_nested_input(): """Test that truncated_graph_inputs does not return repeated inputs.""" x = MyVariable(1) x.name = "x" y = MyVariable(1) y.name = "y" trunc_inp = MyOp(x, y) trunc_inp.name = "trunc_inp" o1 = MyOp(trunc_inp, trunc_inp, x, x) o1.name = "o1" assert truncated_graph_inputs([o1], [trunc_inp]) == [x, trunc_inp, y] o2 = MyOp(x, x, trunc_inp, trunc_inp) o2.name = "o2" assert truncated_graph_inputs([o2], [trunc_inp]) == [trunc_inp, x]It seems to me it should return [x, y]?
There was a problem hiding this comment.
Order is graph traversal dependent. Closer nodes depth first appear first
There was a problem hiding this comment.
The correct should not include y
There was a problem hiding this comment.
A subgraph must have independent inputs to be valid. trunc_inp depends on x, so they can't both be inputs? Or am I thinking wrong here about what truncated_inputs should mean?
There was a problem hiding this comment.
trunc_inp is declared as ancestors_to_include, therefore, other added variables should not depend on them. Yes, trunc_inp depends on x but not vice versa and x is absolutely valid to include. All the above dependencies are assumed to be totally ignored, treat as setting var.owner=None. Logic should be
- Include
ancestors_to_includeanyway, but exclude disconnected - Include the rest variables eagerly as soon as they do not depend on
ancestors_to_include
There was a problem hiding this comment.
This means we need to check candidates in topological order no?
There was a problem hiding this comment.
candidates start with outputs, not ancestors_to_include, which I did not sort topologically, thus can affect now
There was a problem hiding this comment.
What affected in your test case is the order op.inputs, which is not topological, right?
There was a problem hiding this comment.
I did not realise how non topological order of op.inputs can break the set of outputs yet...
ricardoV94 added help wanted 
It could return duplicated truncated inputs before the changes, as well as return wrong outputs based on the nodes input order
ricardoV94 force-pushed the fix_truncated_graph_inputs_bug branch from f53e356 to 16c28c6 Compare 
| @ricardoV94 once you feel finished, you can merge |
2 participants
Follow up to #66