Make ngraph_bridge::Node members const where possible #69

adstraw · 2017-11-06T18:09:03Z

Refactor emitter tests according to above const change
Make ngraph_bridge::Node constructors protected
Use more explicit names for function arguments for readability
ngraph_graph.h and .cc clang-format Google

Refactor emitter tests according to above const change Make ngraph_bridge::Node constructors protected Use more explicit names for function arguments for readability ngraph_graph.h and .cc clang-format Google

mbrookhart · 2017-11-06T18:17:03Z

tests/cpp/ngraph/test_ngraph_emitter.h

@@ -25,11 +25,11 @@ struct testEmitter : public Emitter {
  NgraphNodePtr data1;
  NgraphNodePtr data2;
  NgraphNodePtr data3;
-  testEmitter() {
+  testEmitter(nnvmNodePtr n) {


Why add the nnvmNodePtr to the test constructor when it isn't used?

Nevermind, found the usecase with Adam's help.

adstraw

I ran clang-format on ngraph_graph.h and cc files so I annotated with comments to help navigate those diffs.

adstraw · 2017-11-06T18:12:56Z

src/ngraph/ngraph_graph.cc

    for (auto i : n->inputs_) {
-      if (i->name_ == "") i->name_ = randomString(6);
-      if (n->name_ == "") n->name_ = randomString(6);


I ran clang-format on this file and most of the changes are due to clang-format (non-functional). So, adding some comments to draw attention to the functional changes.

adstraw · 2017-11-06T18:14:10Z

src/ngraph/ngraph_graph.cc

@@ -250,7 +240,7 @@ void Graph::CollapseSubgraphs() {
  // loop variable for undefined number of subgraphs
  int i = 1;
  while (true) {
-    auto tmpGraph = std::make_shared<Graph>();
+    auto tmpGraph = std::make_shared<Graph>("subgraph_" + std::to_string(i));


moved Graph name creation to Graph construction time to allow const name

adstraw · 2017-11-06T18:14:42Z

src/ngraph/ngraph_graph.cc

@@ -265,7 +255,6 @@ void Graph::CollapseSubgraphs() {
      // set node name and shape based on last node in the subgraph
      auto name = tmpGraph->nodes_.back()->name_;
      auto shape = tmpGraph->nodes_.back()->shape_;
-      tmpGraph->name_ = "subgraph_" + name + "_" + randomString();


... instead of setting on name inside the loop

adstraw · 2017-11-06T18:17:19Z

src/ngraph/ngraph_graph.h

@@ -42,17 +43,23 @@ using nnvmNodePtr = std::shared_ptr<nnvm::Node>;
 using NodePtr = std::shared_ptr<Node>;

 // Possible Types of nodes in Current Version
-enum class NodeType {kVariable, kAux, kOp, kGraph};
+enum class NodeType { kVariable, kAux, kOp, kGraph };


A few changes in this file due to clang-format but mostly these are "real" changes

ransford2011

Overall looks good. Would like to hear your feedback on the comments I provided please.

ransford2011 · 2017-11-06T18:50:19Z

src/ngraph/ngraph_graph.h

-  Node(NodeType t, const nnvmNodePtr n, std::string s, std::vector<NodePtr> i)
-      : type_(t), orig_node_(n), name_(s), inputs_(i){};
+ protected:
+  Node(NodeType type, nnvmNodePtr node, const std::string& name)


Instead of the logic on line 54, couldn't you just give it a default parameter value?

I can, but if I do I would likely want to have a default parameter for the inputs_ argument as well. This would mean collapsing to a single ctor per class. I have mixed feelings about this. What do other people think?

I guess the default value for the inputs in the first constructor is just an empty vector, I think it would be okay to create 1 constructor.

If you change the ordering, you would not. Let's not dwell on it though. Having conditional within the initializer lists does slow down construction though.

ransford2011 · 2017-11-06T18:54:40Z

src/ngraph/ngraph_graph.h

 };

 // Node for storing operations
 class OpNode : public Node {
 public:
  // Include operation in graphviz
-  std::string createNodeLabel() {
+  std::string createNodeLabel() override {


I think this needs to be virtual as well. I could be wrong here, but I will double check.

Yes. I double-checked. This needs to be virtual

virtual and override are mutually exclusive, per my understanding. virtual => the function can be overridden by a child class. override => the function is overriding a virtual function from the parent class. please send your documentation. Here's mine:

http://en.cppreference.com/w/cpp/language/override

Note that B::foo() is not marked virtual.

@adstraw kinda.. if I remember correctly override is complementary to virtual. It's a hint to a compiler that you actually want to override a virtual method. If you make a mistake and use it on a non-virtual method the compiler will get upset. You don't have to use override on a virtual method you would like to override but it's considered a good practice.

My documentation is in the form of slides and is called Information Hiding. You can find the explanation in Effective C++. I will concede to your documentation for now as it seems to be fine. I like en.cppreference.com.

@Krovatkin That is the way I understand it as well. Non-virtual functions are binded statically to objects. Thus I'm not sure if the proper function will be called. A unit test would resolve this debate 😄

@ransford2011 it should do the right thing AFAIK. override is kinda "superset" of virtual in a sense it adds virtual to the declaration to which override was applied to and it also checks that the method in a base class is indeed virtual so it can be overridden. I'm used to a combo of virtual/override I didn't realize the standard also allows me to just use override.

As usual, C++ takes a neat idea and makes it confusing. In c# if you use override and virtual the compiler will complain, so there's just one way in which override is allowed to be used.

@ransford2011 @adstraw

#include <iostream> struct A { virtual void v() { std::cout << "A::v" << std::endl; }; void m() { std::cout << "A::m" << std::endl; }; }; struct B : public A { virtual void v() override { std::cout << "B::v" << std::endl; }; //void m() override {}; ERROR : overrides non-virtual method virtual void m() { std::cout << "B::m" << std::endl; }; //probably doesn't do what a user wanted }; struct C : public A { void v() override { std::cout << "C::v" << std::endl; }; }; int main() { A* a = new A(); A* b = new B(); A* c = new C(); a->v(); //A::v b->v(); //B::v c->v(); //C::v a->m(); //A::m b->m(); //A::m oopsie return 0; }

😆 . Thanks for running that small experiment. As @adstraw has shown from his reference, the behavior is as intended. That's good for me to know as I can save some keystrokes from typing virtual all the time. Thanks again!

Ransford you didn't delete a,b,c in your code. Memory leak! Could you please fix that??? :)

Seriously though... Thanks for following up!

adstraw · 2017-11-06T22:54:58Z

hold off on merge. I missed a unit test failure ngraph_graph.graph_init. will take care of that now.

adstraw · 2017-11-06T23:51:00Z

fixed test issue... can't check for empty string when using default Graph ctor now that the name is randomized.

Make ngraph_bridge::Node members const where possible

68be487

Refactor emitter tests according to above const change Make ngraph_bridge::Node constructors protected Use more explicit names for function arguments for readability ngraph_graph.h and .cc clang-format Google

adstraw requested review from louisfeng, mbrookhart, Krovatkin and ransford2011 November 6, 2017 18:09

mbrookhart approved these changes Nov 6, 2017

View reviewed changes

adstraw commented Nov 6, 2017

View reviewed changes

ransford2011 reviewed Nov 6, 2017

View reviewed changes

ransford2011 approved these changes Nov 6, 2017

View reviewed changes

Fix graph_init test which was failing for random init graph name

b95f5ec

mbrookhart merged commit eb2b00f into ngraph-integration-dev Nov 7, 2017

mbrookhart deleted the adstraw/node-graph-refactor branch November 7, 2017 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ngraph_bridge::Node members const where possible #69

Make ngraph_bridge::Node members const where possible #69

adstraw commented Nov 6, 2017

mbrookhart Nov 6, 2017

mbrookhart Nov 6, 2017 •

edited

Loading

adstraw left a comment

adstraw Nov 6, 2017

adstraw Nov 6, 2017

adstraw Nov 6, 2017

adstraw Nov 6, 2017

ransford2011 left a comment

ransford2011 Nov 6, 2017

adstraw Nov 6, 2017

mbrookhart Nov 6, 2017

ransford2011 Nov 6, 2017

ransford2011 Nov 6, 2017

ransford2011 Nov 6, 2017

adstraw Nov 6, 2017

Krovatkin Nov 6, 2017

ransford2011 Nov 6, 2017

ransford2011 Nov 6, 2017 •

edited

Loading

Krovatkin Nov 6, 2017

Krovatkin Nov 6, 2017

ransford2011 Nov 6, 2017

adstraw Nov 6, 2017

adstraw commented Nov 6, 2017

adstraw commented Nov 6, 2017

Make ngraph_bridge::Node members const where possible #69

Make ngraph_bridge::Node members const where possible #69

Conversation

adstraw commented Nov 6, 2017

Choose a reason for hiding this comment

mbrookhart Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

adstraw 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ransford2011 Nov 6, 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

adstraw commented Nov 6, 2017

adstraw commented Nov 6, 2017

mbrookhart Nov 6, 2017 •

edited

Loading

ransford2011 Nov 6, 2017 •

edited

Loading