New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix common_subexpression bug #1338

Merged
merged 12 commits into from Dec 3, 2015

Conversation

Projects
None yet
2 participants
@cpcloud
Member

cpcloud commented Dec 3, 2015

Fixes #1325

@cpcloud cpcloud self-assigned this Dec 3, 2015

@cpcloud cpcloud added this to the 0.9.0 milestone Dec 3, 2015

@cpcloud cpcloud added the bug label Dec 3, 2015

@cpcloud

This comment has been minimized.

Member

cpcloud commented Dec 3, 2015

@llllllllll this could use your review when you get a chance

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 3, 2015

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))

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

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.

This comment has been minimized.

@cpcloud

cpcloud Dec 3, 2015

Member

Good point, I'll make this change

['e', 'g', 'a']
"""
common = set(a) & set(b)
return [x for x in unique(concat((a, b))) if x in common]

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

could we make this lazy by default by returning a genexpr? Also maybe move to utils since this seems generally useful

@property
def _args(self):
# XXX: is this a hack?

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

Why do you think this might be a hack?

This comment has been minimized.

@cpcloud

cpcloud Dec 3, 2015

Member

because _args is supposed to be the values of __slots__ sans the _hash property and _full_expr isn't a slot

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

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?

This comment has been minimized.

@cpcloud

cpcloud Dec 3, 2015

Member

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

This comment has been minimized.

@llllllllll

llllllllll Dec 3, 2015

Member

Ah, okay. Lets leave the comment for now then and maybe revisit the path algorithm some time in the future.

This comment has been minimized.

@cpcloud

cpcloud Dec 3, 2015

Member

IOW, _children[0].isidentical(_scalars[0]) is False because _children datashapes have the dimension whereas _scalars don't

@llllllllll

This comment has been minimized.

Member

llllllllll commented Dec 3, 2015

Looks great!

cpcloud added a commit that referenced this pull request Dec 3, 2015

Merge pull request #1338 from cpcloud/transform-subexpr
Fix common_subexpression bug

@cpcloud cpcloud merged commit 66757ed into blaze:master Dec 3, 2015

@cpcloud cpcloud deleted the cpcloud:transform-subexpr branch Dec 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment