Fix common_subexpression bug #1338
Conversation
|
@llllllllll this could use your review when you get a chance |
|
I will look at this now |
| all_leaves = [expr._leaves() for expr in exprs] | ||
|
|
||
| # leaves common to all expressions | ||
| leaves = set.intersection(*map(set, all_leaves)) |
There was a problem hiding this comment.
this fails if there are no exprs, one way to fix this is to define the function like:
def common_subexpression(expr, *exprs):
exprs = (expr,) + exprs
...This will make it raise the correct type error if the one required argument is not passed.
There was a problem hiding this comment.
Good point, I'll make this change
|
|
||
| @property | ||
| def _args(self): | ||
| # XXX: is this a hack? |
There was a problem hiding this comment.
Why do you think this might be a hack?
There was a problem hiding this comment.
because _args is supposed to be the values of __slots__ sans the _hash property and _full_expr isn't a slot
There was a problem hiding this comment.
I thought _args was supposed to be all of the arguments passed to the constructor, is there a reason we need to put the args of _fullexpr in there?
There was a problem hiding this comment.
we need because otherwise there's no path from a broadcast expression to the true leaf because for the following expression t.amount + 1, Broadcast's _args look like this _children=(t,), _scalars=(t,), _scalar_expr=t.amount + 1) and the path algorithm will traverse an expression until we've reached the end of the path and in this case we are traversing to a leaf which doesn't appear in Broadcast._traverse() because everything is scalar-ified
There was a problem hiding this comment.
Ah, okay. Lets leave the comment for now then and maybe revisit the path algorithm some time in the future.
There was a problem hiding this comment.
IOW, _children[0].isidentical(_scalars[0]) is False because _children datashapes have the dimension whereas _scalars don't
|
Looks great! |
Fixes #1325