Improve Remove Flatten
Merge request reports
Activity
assigned to @cmoineau
added Enhancement ⭐ label
mentioned in issue #159 (closed)
39 40 40 void removeFlatten(std::shared_ptr<GraphView> graphView){ 41 42 43 std::shared_ptr<GraphRegex> regex = std::make_shared<GraphRegex>(); 44 regex->setNodeKey("Flatten","getType($) =='Flatten'"); 45 regex->setNodeKey("FC","getType($) =='FC'"); 46 regex->addQuery("Flatten->FC"); 41 const auto matches = SinglePassGraphMatching(graphView).match( 42 "(FC|MatMul)<-(Flatten)+" 43 ); 47 44 48 for (const auto& solution : regex->match(graphView)) { 49 removeFlatten(solution); 45 for (const auto& solution : matches) { 46 removeFlatten(solution.graph->getNodes()); Suggestion: replace the
removeFlattenfunction with 2 lines:auto flattenNodes(solution.graph->getNodes()); flattenNodes.erase(solution.graph->rootNode()); GraphView::replace(flattenNodes, {});The root node is garanteed to be FC or MatMul, the other nodes are garanteed to be Flatten node.
Why is the root node guaranteed to be FC or MatMul ? From the documentation:
There is a root node in the GraphView (by default, the first node added to it), and nodes inside a GraphView can be ranked with a unique and deterministic order if the nodes form a single connected graph;
So is it because by construction of the GraphMatching algorithm, the graph is built with nodes declared from left to right in the query ? If so is this behavior documented ?
Also, I am not in favor of using this mechanism has I find that it limit the readability of the code. If we use the proposed implementation, I think a explanation comment would be required.
Yep, it is documented: "The first node of the first sequence is the root node and cannot be optional" (by construction in the GraphMatching). It is an important feature.
Edited by Olivier BICHLERThe implementation regarding root node will not change in the future. It is an important feature of graph matching and I already rely on it heavily. I would prefer to keep the proposed suggestion, perhaps with the right comment, as I find it more idiomatic of how graph matching should be used (meaning, ideally, without having to re-iterate through the matched nodes and check their type).
Also, there is no purpose of keeping a sub-function anymore...
Edited by Olivier BICHLERchanged this line in version 5 of the diff
added 11 commits
-
20abe188...f410ee54 - 9 commits from branch
dev - 4bba1745 - Merge branch 'dev' into ImproveRemoveFlatten
- 441b4bde - Remove old GraphRegex to use new GraphMatching in unit test.
-
20abe188...f410ee54 - 9 commits from branch
requested review from @olivierbichler
added 32 commits
-
04231e35...7f1ba66c - 27 commits from branch
dev - a551865e - Remove Flatten now use new graphmatching + remove Flatten beofre MatMul + remove multiple Flatten.
- b1097ce8 - Remove old GraphRegex to use new GraphMatching in unit test.
- 2659ce34 - Fixed GraphMatching
- 7f304045 - Update remove flatten tests.
- 515ef7d5 - Applied proposed change in...
Toggle commit list-
04231e35...7f1ba66c - 27 commits from branch
enabled an automatic merge when the pipeline for 515ef7d5 succeeds
mentioned in commit 45cd45cc
