Implementation of cansplitpush, repeat_operator, replace_and_remove_nodes and find_lowest_nodes #245
Conversation
|
@kylebd99 I have changed the codebase as requested. |
kylebd99
left a comment
kylebd99
left a comment
There was a problem hiding this comment.
Hey, sorry for the slow review, I think everything looks good. I had a couple of very minor comments, but I'm going to go ahead and approve it. Please make the changes, then go ahead and merge.
| return components | ||
|
|
||
|
|
||
| def replace_and_remove_nodes( |
There was a problem hiding this comment.
This function should iterate through the full pointwise expression tree rather than just operating on one node. In the julia implementation, you can see this by noting that it does a PostOrderDFS through the pointwise expression https://github.com/finch-tensor/Finch.jl/blob/5dff68a19d3c480358b5e0ac112af7e856ba2b73/src/Galley/LogicalOptimizer/annotated-query.jl#L448.
| assert result == expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
For this test, rather than giving a set of arg names. I would make all of the inputs FinchLogicNodes, and I would make sure that you have a test which involves a pointwise expression that has some level of nesting.
| ["A"], | ||
| ), | ||
| # Nested case: | ||
| # root = MapJoin(mul, [A(i,j), B(i)]), reduce over i → [A] |
There was a problem hiding this comment.
Nit: I think you mean B(j) in this comment.
There was a problem hiding this comment.
Further, I'd appreciate 1 additional nested case where B has the index "i" which returns the mapjoin as the lowest root.
Hi! I have implemented cansplitpush, repeat_operator, replace_and_remove_nodes and find_lowest_nodes and have written test for every one of the functions. I have changed the backbone of annotated_query to make its semantic clearer. I have also file an issue for each function.
Fix #241, Fix #242, Fix #243 & Fix #244
Thank you for your time!!