Distributed v2 - #65
Conversation
inducer
left a comment
There was a problem hiding this comment.
Some thoughts on your GraphToDictMapper implementation below.
| return sum([a for a in args]) | ||
|
|
||
|
|
||
| class GraphToDictMapper(WalkMapper): |
There was a problem hiding this comment.
Are you benefiting from being a WalkMapper here? You need to override all map_ methods here anyhow, and if you miss one, you get incorrect results.
There was a problem hiding this comment.
My impression when using the Mapper class was that I was just reimplementing a WalkMapper, but considering your other comment below, a Mapper seems more appropiate.
| children = children | {expr} | ||
| super().map_placeholder(expr, children) |
There was a problem hiding this comment.
Wouldn't this construction end up gathering up all the children of a node, not just the immediate ones?
There was a problem hiding this comment.
Yes, but I thought that was the goal :-/. Will fix.
There was a problem hiding this comment.
Should be fixed now, could you check again @inducer ?
There was a problem hiding this comment.
Yes, but I thought that was the goal :-/. Will fix.
It can't be: That would be a data structure of size O(n^2)!
|
Unsubscribing... @-mention or request review once it's ready for a look or needs attention. |
| :class:`dict`, maps each node in the graph to the set of directly connected | ||
| nodes. |
|
Closed in favor of #148. |
cc @matthiasdiener