[Fix] eliminate_two_op_chain breaks DAG when second op has multiple outputs (#70)#71
[Fix] eliminate_two_op_chain breaks DAG when second op has multiple outputs (#70)#71ArthurPendragn wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e-strauss
left a comment
e-strauss
left a comment
There was a problem hiding this comment.
thanks @ArthurPendragn , the fix looks good, also thanks for adding the additional test cases. I only have some minor comments.
| else: | ||
| x.outputs = [] | ||
| replace_op_in_outputs(op2, x) | ||
| x.outputs = [out for out in x.outputs if out is not op1] |
There was a problem hiding this comment.
nice finding, i forgot about the util method. can we change the order of line 21 and 22?
so first eliminate op1 as output of x, than update op2 outputs (and implicit add outputs to x), since we should do the copy of the list before we append op2 outputs
| df_ops = [op for op in linearized_dag if isinstance(op, ValueOp) and op.value == 1.0] | ||
| a_op = next(op for op in df_ops if len(op.outputs) > 1) |
There was a problem hiding this comment.
i think is okay to use simply linearized_dag[0] here since the ValueOp should be always the first operator, since every other op depends on it. maybe with an assertion for isinstance(op, ValueOp) and op.value == 1.0]
| df_ops = [op for op in linearized_dag if isinstance(op, ValueOp) and op.value == 1.0] | ||
| a_op = df_ops[0] |
| """ | ||
| Scenario: | ||
| a -> [log, d] | ||
| log -> [exp] (root) |
There was a problem hiding this comment.
the exp op is not root here, isn't it here:
exp -> BinOp (root)
or am I wrong?
| df_ops = [op for op in linearized_dag if isinstance(op, ValueOp) and op.value == 1.0] | ||
| a_op = next(op for op in df_ops if len(op.outputs) > 0) |
ArthurPendragn
force-pushed
the
twoOpChainFix-70
branch
from
e1d5a0a to
064f5a9
Compare
ArthurPendragn
force-pushed
the
twoOpChainFix-70
branch
from
064f5a9 to
f6cdeff
Compare
2 participants
Closes #70