AutoBroadcast class with broadcast_add first usage #57

adstraw · 2017-10-20T16:51:39Z

No description provided.

mbrookhart

Overall, I think it's looking great! There are a few sanity checks/reference changes I'd like, but over all I think it's pretty much there.

mbrookhart · 2017-10-20T17:05:27Z

src/ngraph/ngraph_autobroadcast.cc

+    SetShapesAndAxes();
+
+    // if auto broadcast is possible
+    if (broadcastshape_.size()) {


Can we add a sanity check that broadcastshape_ is equal to the output node shape?

not sure I understand. this is the job of the unit tests, in my opinion. but, I may be missing your point. if I want to check that broadcast shape is equal to output shape don't I need to have type propagation inside the class?

Let me rephrase: Mxnet has inferred what the output of this broadcast should be. We're doing a second inference to identify how to expand the axes. Should we validate that mxnet's inferred shape and autobroadcast's inferred shape are the same?

I did not end up doing this. Was wondering... do we actually need to check for this error or will it be caught by some other piece of code. Let me know.

I don't think this will actually show an error, but I'm paranoid ;) If it fails for some obscure test case, it will through an error somewhere, but that error might be hard to debug. It's okay for now.

if I purposely mess up broadcastshape by adding '2' as a leading dimension after exiting the loop I get:

terminate called after throwing an instance of 'std::invalid_argument'
what(): Error with node Node(Broadcast_2): Broadcast arg, shape, and axes are incompatible
Aborted (core dumped)

@adstraw @mbrookhart , I second adding an assertion. What @adstraw is describing is the current implementation. Imagine, Alice comes in and makes some changes to SetShapesAndAxes so this error isn't produced Error with node Node(Broadcast_2): and yet the shapes are indeed different (that the promise SetShapesAndAxes is required to fulfill). In debug builds we would like to fail as early as possible to the source of a problem and asserting that the shapes indeed match is a way to confirm that SetShapesAndAxes fulfills its promise.

I am leaving this out, for now. If we find that we crash and burn at some later date we can add this check. As it stands, AutoBroadcast produces no errors which simplifies things. It simply 1) broadcasts if possible or 2) leaves things as-is and punts error handling down the road.

mbrookhart · 2017-10-20T17:09:13Z

tests/cpp/ngraph/test_ngraph_autobroadcast.cc

+  EXPECT_EQ(getShapeFromParam(ab.rhs()), s1345);
+}
+
+}  // namespace ngraph_bridge


These tests look great, thank you!

mbrookhart · 2017-10-20T17:09:17Z

src/ngraph/ngraph_emitter.cc

+    auto rhsShape = TShape_to_NShape(node->inputs[1]->shape);
+
+    AutoBroadcast ab(lhsNode, lhsShape, rhsNode, rhsShape);
+    return ab.lhs() + ab.rhs();


I'm still confused by the object that's simply a constructor and two getters, but that's a longer term discussion and doesn't block this.

No test for this function? I'm worried that this will fail since ab.lhs() and ab.rhs() are const references to shared pointers only defined in the AutoBroadcast...they will be destroyed when ab is when this function returns, then the returned ngraph node will have nullptrs in it?

simply forgot the test for broadcast_add, will do that now

I'm still confused by the object that's simply a constructor and two getters, but that's a longer term discussion and doesn't block this.

It's actually a really nice API. It's impossible to misuse 😄

mbrookhart · 2017-10-20T17:10:26Z

src/ngraph/ngraph_autobroadcast.cc

+        node.ptr, broadcastshape_, node.axes);
+  }
+}
+


I lean towards minimal comments, I find they clutter the code and make it harder for me to read. I find this over-commented, but that may just be me, and if everyone else wants more comments, that's okay.

I deleted a few comments, hope it's better

mbrookhart · 2017-10-20T17:11:59Z

src/ngraph/ngraph_autobroadcast.h

+  AutoBroadcast(const NgraphNodePtr &lhsNode, const ngraph::Shape &lhsShape,
+                const NgraphNodePtr &rhsNode, const ngraph::Shape &rhsShape);
+  const NgraphNodePtr &lhs() { return lhs_.ptr; }
+  const NgraphNodePtr &rhs() { return rhs_.ptr; }


I think this should return a copy of the pointer, not a reference. I'm worried that the shared pointer will go out of scope and delete the node between construction and getting called in the graph executor.

mbrookhart

LGTM, thank you!

mbrookhart · 2017-10-20T20:11:07Z

src/ngraph/ngraph_autobroadcast.cc

+    SetShapesAndAxes();
+
+    // if auto broadcast is possible
+    if (broadcastshape_.size()) {


I don't think this will actually show an error, but I'm paranoid ;) If it fails for some obscure test case, it will through an error somewhere, but that error might be hard to debug. It's okay for now.

Krovatkin · 2017-10-21T20:52:17Z

src/ngraph/ngraph_autobroadcast.cc

+      lhs_.reshape.insert(lhs_.reshape.begin(), lhsDim);
+      rhs_.reshape.insert(rhs_.reshape.begin(), rhsDim);
+
+    } else if (rhsDim == 1) {


rhsDim == 1 and lhsDim == 1 cases seem to duplicate the same logic. Would it make sense to add a small helper and call it like in this snippet below?

if (rhsDim == 1) { collectBroadcastAndReshapeAxes(lhsDim, rhsDim, lhs_, rhs_); } else (lhsDim == 1) { collectBroadcastAndReshapeAxes(rhsDim, lhsDim, rhs_, lhs_); }

saw this after my last patch. I agree - it could clean up the code. let's see if I get more feedback and I can address.

Krovatkin · 2017-10-23T16:27:44Z

tests/cpp/ngraph/test_ngraph_autobroadcast.cc

+// basic reshape and broadcast test
+// rhs reshape to 2,3,4 then
+// rhs broadcast to 2,3,4,5
+TEST(NGRAPH_AUTOBROADCAST, RESHAPE_1X_BROADCAST) {


In future, we could probably add a few more cases:

scalar -> vector

scalar -> matrix

vector -> matrix

Krovatkin · 2017-10-23T16:30:47Z

tests/cpp/ngraph/test_ngraph_emitter.h

+    data1 = op_map[in1];
+    data2 = op_map[in2];
+  };
+};
 }


nitpick: no new line

Krovatkin · 2017-10-23T16:34:40Z

src/ngraph/ngraph_autobroadcast.h

+    ngraph::Shape reshape;
+    // axes (0-based) to broadcast by ngraph::op::Broadcast
+    ngraph::AxisSet axes;
+  } lhs_, rhs_;


an off-topic question? is this the convention we are using for members (members' names end w/ underscores)?

My opinion: It seems to be the mxnet guideline therefore it is our guideline.

It is standard practice so I think that we should follow that. It is something I've put into the Coding guideline document. I would say that it's pretty conventional at this point in C++.

in my previous gig the standard was m_camelCaseVariable. I don't really care, just so long as we have a plan.

for basic broadcast 2D and 3D cases also to handle edge input cases (empty, zero dimension)

Krovatkin · 2017-10-23T20:18:58Z

src/ngraph/ngraph_autobroadcast.cc

+  // a zero dimension is invalid
+  // so we should not hit this case "in the wild"
+  // make explicit: no action taken on shapes with zero dimensions
+  if (std::find(lhs_.shape.begin(), lhs_.shape.end(), 0) != lhs_.shape.end())


Should we turn these into assertions? If it's indeed invalid input and we never expect to see such shapes that's what we should assert. OTOH, if graphs w/ such shapes are valid but they don't make sense to broadcast we should throw, instead. What do you guys think @adstraw @mbrookhart

good point. I don't see any other case in src/ngraph where we are using assert. @mbrookhart is there any reason not to use assert in this case?

Krovatkin · 2017-10-23T20:20:50Z

src/ngraph/ngraph_autobroadcast.cc

+  // mxnet scalars are pre-broadcast to requisite shape
+  // so we should not hit this case "in the wild"
+  // make explicit: no action taken on empty shape(s)
+  if (lhs_.shape.size() == 0 || rhs_.shape.size() == 0) return false;


again, maybe this should be an assertion? my arguments are in the above comment

ransford2011

I'm pretty late on this code review. I just added my comments on this but do not want to be the one to hold up the check-in.

ransford2011 · 2017-10-23T20:12:58Z

src/ngraph/ngraph_autobroadcast.cc

+  if (node.shape != node.reshape) {
+    // tell reshape to examine input dimensions in order
+    ngraph::AxisVector order(node.shape.size());
+    std::iota(order.begin(), order.end(), 0);


Using STL algorithms!! Nice!!! 👍 💯

ransford2011 · 2017-10-23T20:14:31Z

src/ngraph/ngraph_autobroadcast.cc

+                             const ngraph::Shape &lhsShape,
+                             const NgraphNodePtr &rhsNode,
+                             const ngraph::Shape &rhsShape) {
+  lhs_.ptr = lhsNode;


These should be initialized using the initializer list. It's faster. I'm also not a big fan of having so much logic in the constructor, but I understand there is precedence of that in our code already. Constructors should at most be initializing the basic components for having an object instantiated.

good point. code is now merged. I can fix in a future patch.

ransford2011 · 2017-10-23T20:18:21Z

src/ngraph/ngraph_autobroadcast.h

+    ngraph::Shape reshape;
+    // axes (0-based) to broadcast by ngraph::op::Broadcast
+    ngraph::AxisSet axes;
+  } lhs_, rhs_;


It is standard practice so I think that we should follow that. It is something I've put into the Coding guideline document. I would say that it's pretty conventional at this point in C++.

ransford2011 · 2017-10-23T20:18:58Z

src/ngraph/ngraph_autobroadcast.h

+  //       e.g. when adding (2,3) tensor A to (2,1) tensor B
+  //            first Reshape tensor B to (2)
+  //            then Broadcast tensor B to (2,3)
+  void ReshapeAndBroadcast(Node &node);


No const on the parameter???

in this case we are actually modifying the node within the function so we want a non-const reference here.

ransford2011 · 2017-10-23T20:22:41Z

src/ngraph/ngraph_autobroadcast.cc

+  // a zero dimension is invalid
+  // so we should not hit this case "in the wild"
+  // make explicit: no action taken on shapes with zero dimensions
+  if (std::find(lhs_.shape.begin(), lhs_.shape.end(), 0) != lhs_.shape.end())


I'm a fan of the STL algorithm usage 🥇

adstraw added 2 commits October 20, 2017 08:55

add AutoBroadcast class and unit tests

691aebd

add broadcast_add as first user of AutoBroadcast

cd809d3

adstraw requested review from Krovatkin, sasadep, ransford2011 and mbrookhart October 20, 2017 16:55

mbrookhart suggested changes Oct 20, 2017

View reviewed changes

add unit test for broadcast_add

5494b6d

mbrookhart reviewed Oct 20, 2017

View reviewed changes

mbrookhart approved these changes Oct 20, 2017

View reviewed changes

Krovatkin approved these changes Oct 23, 2017

View reviewed changes

add some AutoBroadcast unit tests

6299533

for basic broadcast 2D and 3D cases also to handle edge input cases (empty, zero dimension)

Krovatkin reviewed Oct 23, 2017

View reviewed changes

ransford2011 approved these changes Oct 23, 2017

View reviewed changes

sasadep approved these changes Oct 23, 2017

View reviewed changes

mbrookhart merged commit fdfc509 into ngraph-integration-dev Oct 23, 2017

adstraw deleted the adstraw/auto-broadcast branch October 26, 2017 20:18

AutoBroadcast class with broadcast_add first usage #57

AutoBroadcast class with broadcast_add first usage #57

Conversation

adstraw commented Oct 20, 2017

mbrookhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookhart Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookhart left a comment

Choose a reason for hiding this comment

mbrookhart Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

Krovatkin Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Krovatkin Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

ransford2011 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookhart Oct 20, 2017 •

edited

Loading

mbrookhart Oct 20, 2017 •

edited

Loading

Krovatkin Oct 21, 2017 •

edited

Loading

Krovatkin Oct 23, 2017 •

edited

Loading

Krovatkin Oct 23, 2017 •

edited

Loading

Krovatkin Oct 23, 2017 •

edited

Loading