-
Notifications
You must be signed in to change notification settings - Fork 135
Fix bug in infer_shape of Blockwise(Subtensor) #1353
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -264,9 +264,13 @@ class TestOpWithInferShape(Op): | |
def make_node(self, a, b): | ||
assert a.type.ndim == 1 | ||
assert b.type.ndim == 1 | ||
# Simulate make_node that introduces operations on inputs | ||
a_identity = a.copy() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does copying the input simulate an operation on the inputs? The apply takes the copies as inputs, so any intermediate operations (the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After rewrites, but the Blockwise.infer_shape acts on the returned graph immediately. The thing Blockwise is trying to is to figure out if the the core_shape of the Op depends on the values of the inputs, or can be guessed just based on their shapes. For that it calls If they are not used in the shape of the graph, then it means only the core shape is needed which is fine to use. This would be the case of a Blockwise(Dirichlet), where only the core shape (length of alpha), but not their value is needed. (We don't blockwise RVs but you get the idea). Anyway, the logic to figure out if the core values are needed was to create a dummy node and then checking if the node inputs were in the graph. But this failed when the dummy node didn't really use the dummy variables (because it added extra nodes, like DimShuffle or ScalarFromTensor in the case that actually failed). Identity here is an easy way to test this without having to change anything else in the test. |
||
b_identity = b.copy() | ||
|
||
c = tensor(shape=(None,)) | ||
d = tensor(shape=(None,)) | ||
return Apply(self, [a, b], [c, d]) | ||
return Apply(self, [a_identity, b_identity], [c, d]) | ||
|
||
def perform(self, node, inputs, outputs): | ||
a, b = inputs | ||
|
@@ -277,9 +281,12 @@ def perform(self, node, inputs, outputs): | |
def infer_shape(self, fgraph, node, input_shapes): | ||
# First output shape depends only on input_shapes | ||
# Second output shape depends on input values | ||
x, y = node.inputs | ||
[(x_shape,), (y_shape,)] = input_shapes | ||
return (x_shape + y_shape,), (x.sum() + y.sum(),) | ||
a_identity, b_identity = node.inputs | ||
# Simulate shape depending on original inputs, not the ones that go directly into the node | ||
a = a_identity.owner.inputs[0] | ||
b = b_identity.owner.inputs[0] | ||
[(a_shape,), (b_shape,)] = input_shapes | ||
return (a_shape + b_shape,), (a.sum() + b.sum(),) | ||
|
||
blockwise_op = Blockwise( | ||
core_op=TestOpWithInferShape(), signature="(a),(b)->(c),(d)" | ||
|
Uh oh!
There was an error while loading. Please reload this page.