Reshape #77
Reshape #77
Conversation
src/ngraph/ngraph_emitter.cc
Outdated
| @@ -176,6 +176,26 @@ void Emitter::CreateUnaryOps() { | |||
| auto oneeighty = makeConstant(node, "180"); | |||
| return op_map_[node->inputs_[0]] * (pi / oneeighty); | |||
| }; | |||
| ngraph_op_funcs_["reshape"] = [this](const NodePtr& node) { | |||
| auto child = op_map_[node->inputs_[0]]; | |||
There was a problem hiding this comment.
Small question. Does Ngraph support the negative shape values that MXNet does? Will that pose a problem? I ask this because typically ngraph return types are size_t which are unsigned. That would screw up the expected values for MXNet.
There was a problem hiding this comment.
@diyessi MxNet runs an analysis called InferShape and the goal of this analysis (at least in case of Reshape) is to produce(normalize/canonicalize) shapes w/ positive dimensions. The inputs that a user supplies can be negative and they define a micro reshape language of sorts:
- ``-1`` infers the dimension of the output shape by using the remainder of the input dimensions
- ``-2`` copy all/remainder of the input dimensions to the output shape.
- ``-3`` use the product of two consecutive dimensions of the input shape as the output dimension.
- ``-4`` split one dimension of the input into two dimensions passed subsequent to -4 in shape (can contain -1).
If the argument `reverse` is set to 1, then the special values are inferred from right to left.
This micro reshape language is "parsed" by InferShape which sets the output shape for a particular reshape operation and this output shape is what we are using.
There was a problem hiding this comment.
Does Ngraph support the negative shape values that MXNet does?
Ngraph doesn't support negative shapes.
Will that pose a problem?
That's a very good question. I suspect that after InferShape runs it normalizes all shapes into shapes w/ positive dimensions but @mbrookhart should be better suited to answer this question.
There was a problem hiding this comment.
NumPy would probably make a negative shape reverse the traversal or something like that, so I think these are pretty MXNet-specific.
For a reshape, we need to know how indices in the pre-shape map to indices in the post-shape and vice versa. The XLA definition was simpler than what we did in Python, particularly in C++.
There was a problem hiding this comment.
I think Shapes are positive definite after MXNet's InferShapes pass, but I can seem to find enforcement of that idea.
There was a problem hiding this comment.
@mbrookhart that's why I was suggesting to add an assertion to TShape_to_NShape. We put it in and move on w/ our lives. If this assertion turns out to be false we go and fix the individual cases that need to be fixed.
There was a problem hiding this comment.
Sure, that's okay with me.
| @@ -48,6 +48,18 @@ inline To convert_shapes(const Ti& inshape) { | |||
| return shape; | |||
| } | |||
|
|
|||
| template <> | |||
There was a problem hiding this comment.
I see that you have made this a template, but it's not really a template. Did you make this a template to avoid a compiler error sir?? 😆
There was a problem hiding this comment.
convert_shapes is already using templates; specializing it for this particular pair <nnvm::TShape, ngraph::Shape> allows us to make sure that the other types won't get this extra error checking.
| EXPECT_EQ(TShape_to_NShape(nnvm::TShape{1}), ngraph::Shape{1}); | ||
| EXPECT_EQ(TShape_to_NShape(nnvm::TShape{2, 3, 4, 5, 6}), | ||
| (ngraph::Shape{2, 3, 4, 5, 6})); | ||
| EXPECT_THROW(TShape_to_NShape(nnvm::TShape{2, 3, 4, -1}), const char*); |
There was a problem hiding this comment.
Nice!! Thanks for adding the test!
There was a problem hiding this comment.
@ransford2011 the more the merrier :-) C++ tests are pretty cheap.
| inline ngraph::Shape convert_shapes(const nnvm::TShape& inshape) { | ||
| ngraph::Shape shape; | ||
| for (const auto& s : inshape) { | ||
| if (s < 0) { |
There was a problem hiding this comment.
Way to go by illustrating Defensive Programming 💯 .
Description
Adding a Reshape peration
Checklist
Essentials
make lint)Changes