Remove ParameterizedConstant #309

rkimballn1 · 2017-12-15T23:32:01Z

Most of the work is on op::Constant. I would like to know what you guys think. If it looks ok then we can bring in more reviewers for the PR but I want to get your opinions before I do more.

diyessi

A few minor scathes that can be resolved by change and/or successful rebuttal.

diyessi · 2017-12-18T17:46:25Z

src/ngraph/autodiff/adjoints.cpp

@@ -35,7 +35,7 @@ using namespace ngraph;
 std::shared_ptr<Node> make_zero(const std::shared_ptr<const TensorViewType>& tensor_view_type)
 {
    std::shared_ptr<Node> zero =
-        std::make_shared<op::Constant>(tensor_view_type->get_element_type(), Shape{}, "0");
+        op::Constant::create(tensor_view_type->get_element_type(), Shape{}, {0.0});


I lean slightly towards calling it make instead of create but that could be because I was doing make-instance before these new-fangled object creation names showed up, so I'll go along with @aprocter .

create sounds good to me.

I kinda lean towards make as well but I figure @rkimballn1 got there first ;)

diyessi · 2017-12-18T18:13:38Z

src/ngraph/ops/constant.cpp

    {
-        throw ngraph_error("Constant does not have tensor view type");
+        for (int value : get_vector<char>())


It would be nice if this big if with the same essential body in each clause pattern I see multiple places could be done without all the duplication, but I don't think C++ templates can do it; you'd need a kind of lambda template for the body.

What about a switch/case instead of this big if?

I remember coming across this when adding polymorphism to NGVM. It looks like templated lambdas are available, but only in C++14.

We have a compiler constraint from MXNet that might prevent this; if not, we should let the compiler let us simplify our sources.

diyessi · 2017-12-18T18:19:08Z

src/ngraph/ops/constant.hpp

+            /// \param values An initializer_list of string values to use as the constant data.
+            template <typename T>
+            static std::shared_ptr<op::Constant>
+                create(const element::Type& type, Shape shape, std::initializer_list<T> values)


Use <typename T> and const T& values and you'll be able to initialize with anything that can initialize a vector.

diyessi · 2017-12-18T18:22:09Z

src/ngraph/types/element_type.hpp

        }
+        template <>


Can you stick spaces between these?

yes, but clang-format removes them.

diyessi · 2017-12-18T18:23:58Z

test/backend_test.in.cpp

@@ -3189,7 +3027,7 @@ TEST(${BACKEND_NAME}, slice_3d_strided_different_strides)
 TEST(${BACKEND_NAME}, scalar_constant_float32)
 {
    auto rt = make_shared<TensorViewType>(element::Float32::element_type(), Shape{});
-    auto r = make_shared<op::Constant>(element::Float32::element_type(), Shape{}, "4.8");
+    auto r = op::Constant::create(element::Float32::element_type(), Shape{}, {4.8});


I wonder if it'd be worthwhile to have an overloading for making scalar constants, like
op::Constant::create(element::Float32::element_type(), 4.8)

diyessi · 2017-12-18T18:26:19Z

test/backend_test.in.cpp

@@ -3201,13 +3039,13 @@ TEST(${BACKEND_NAME}, scalar_constant_float32)
    auto result = backend->make_primary_tensor_view(element::Float32::element_type(), Shape{});

    cf->call({}, {result});
-    EXPECT_EQ(vector<float>{std::strtof("4.8", NULL)}, result->get_vector<float>());
+    EXPECT_EQ(vector<float>{4.8}, result->get_vector<float>());


and similarly, a get_scalar<float> although it might only be unit tests that it would matter for.

aprocter

Looks great! Much cleaner than the string-based stuff. Just a couple small changes requested: some docstrings related, and the "autobroadcasting" constructor (I think that behavior should be attached to a constructor that just takes T rather than vector, rather than to singleton vectors).

aprocter · 2017-12-18T18:58:58Z

src/ngraph/autodiff/adjoints.cpp

@@ -35,7 +35,7 @@ using namespace ngraph;
 std::shared_ptr<Node> make_zero(const std::shared_ptr<const TensorViewType>& tensor_view_type)
 {
    std::shared_ptr<Node> zero =
-        std::make_shared<op::Constant>(tensor_view_type->get_element_type(), Shape{}, "0");
+        op::Constant::create(tensor_view_type->get_element_type(), Shape{}, {0.0});


I kinda lean towards make as well but I figure @rkimballn1 got there first ;)

aprocter · 2017-12-18T19:02:20Z

src/ngraph/ops/constant.cpp

    {
-        throw ngraph_error("Constant does not have tensor view type");
+        for (int value : get_vector<char>())


I remember coming across this when adding polymorphism to NGVM. It looks like templated lambdas are available, but only in C++14.

aprocter · 2017-12-18T19:03:53Z

src/ngraph/ops/constant.hpp

+        /// | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+        /// | `type`          | The ngraph::element::Type of the tensor constant.                                                                                                                              |
+        /// | `shape`         | The ngraph::Shape of the tensor constant.                                                                                                                                      |
+        /// | `value_strings` | A list of strings containing literals for initialization of the tensor constant. These strings are parsed with the appropriate instance of ngraph::element::TraitedType::read. |


No longer strings, right?

aprocter · 2017-12-18T19:04:20Z

src/ngraph/ops/constant.hpp

-            Constant(const element::Type& et,
-                     const Shape& shape,
-                     const std::vector<std::string>& value_strings);
+            /// \param values A vector of string values to use as the constant data.


Again, no longer strings

aprocter · 2017-12-18T19:05:27Z

src/ngraph/ops/constant.hpp

            }

            /// \return The initialization literals for the tensor constant.
-            const std::vector<std::string>& get_value_strings() const { return m_value_strings; }
+            std::vector<std::string> get_value_strings() const;


Is this still used (maybe by the serializer)?

aprocter · 2017-12-18T19:08:36Z

src/ngraph/ops/constant.hpp

            {
+                auto vt = std::make_shared<TensorViewType>(type, shape);
+                set_value_type_checked(vt);
+                if (values.size() == 1)


Looks like a vector length of 1 gives you the "autobroadcasting" behavior. Could we change it so that instead there is a separate constructor for this behavior that doesn't take a vector, like so?

Constant(const element::Type& type, Shape shape, T value)

aprocter · 2017-12-18T19:09:08Z

src/ngraph/runtime/cpu/cpu_emitter.cpp

@@ -480,324 +480,6 @@ void runtime::cpu::CPU_Emitter::EmitSubtract(const ngraph::Node* n,
    m_out << "}\n";
 }

-void runtime::cpu::CPU_Emitter::EmitParameterizedConstantBool(


Sooooo much red ;)

avijit-nervana

Overall looks good and simple. I couldn't try this with TF as currently NGTF is lagging seriously behind the ngraph++ API :-(

avijit-nervana · 2017-12-18T18:52:24Z

src/ngraph/ops/constant.hpp

+        /// | --------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
+        /// | `type`          | The ngraph::element::Type of the tensor constant.                                                                                                                              |
+        /// | `shape`         | The ngraph::Shape of the tensor constant.                                                                                                                                      |
+        /// | `value_strings` | A list of strings containing literals for initialization of the tensor constant. These strings are parsed with the appropriate instance of ngraph::element::TraitedType::read. |


Can we break these long lines into multiple - much easier to read (side by side)

I'll fess up: I'm the one who wrote most of these with super-wide lines in the first place. The embarrassing reason for it is that I could not figure out how to get Doxygen's markdown parser to understand multi-line table cells without explicitly delineating the rows. (i.e. there are two table styles: one where there's one table row per ASCII line, and one where you have to put a bunch of extra dashes between rows.)

Probably switching to the explicitly-delineated rows is the best solution. I could do this in a separate PR since I'm the one who made the mess and I already know how to clean it up.

avijit-nervana · 2017-12-18T18:56:37Z

src/ngraph/autodiff/adjoints.cpp

@@ -35,7 +35,7 @@ using namespace ngraph;
 std::shared_ptr<Node> make_zero(const std::shared_ptr<const TensorViewType>& tensor_view_type)
 {
    std::shared_ptr<Node> zero =
-        std::make_shared<op::Constant>(tensor_view_type->get_element_type(), Shape{}, "0");
+        op::Constant::create(tensor_view_type->get_element_type(), Shape{}, {0.0});


create sounds good to me.

avijit-nervana · 2017-12-18T18:58:08Z

src/ngraph/ops/constant.cpp

    {
-        throw ngraph_error("Constant does not have tensor view type");
+        for (int value : get_vector<char>())


What about a switch/case instead of this big if?

avijit-nervana · 2017-12-18T20:55:04Z

src/ngraph/ops/constant.hpp

+                {
+                    throw std::runtime_error("Constant initializer does not match shape");
+                }
+                if (target_type == element::boolean)


Probably better to replace with a switch/case

avijit-nervana · 2017-12-18T20:59:14Z

src/ngraph/ops/constant.hpp

-                , m_value(value)
+            /// \param values A vector of literals for initializing the tensor constant. The size of values must match the size of the shape.
+            template <typename T>
+            Constant(const element::Type& type, Shape shape, const std::vector<T>& values)


Since you have defined a create(...) method - mark the constructor private?

I'd be a little hesitant to start marking node constructors private unless we make the change consistently. Also make_shared doesn't work with private constructors, even when used inside the class.

avijit-nervana · 2017-12-18T21:02:39Z

src/ngraph/ops/constant.hpp

+
+            template <typename T>
+            std::vector<T> get_vector() const
+            {


Is this needed? If the vector is large, then this will be expensive. If this method doesn't exist then users will have to use the more efficient get_data_ptr() defined below.

avijit-nervana · 2017-12-18T21:04:32Z

src/ngraph/ops/constant.hpp

+            }
+
+            void* get_data_ptr() { return m_data; }
+            virtual bool is_constant() const override { return true; }
        protected:


What other classes are envisioned to be derived from Constant?

avijit-nervana · 2017-12-18T21:08:45Z

src/ngraph/runtime/interpreter/int_call_frame.hpp

@@ -258,7 +258,7 @@ class ngraph::runtime::interpreter::INT_CallFrame : public runtime::CallFrame
        else if (node_op == "Constant")
        {
            auto c = static_cast<const op::Constant*>(&node);
-            std::vector<T> input = ngraph::parse_string<T>(c->get_value_strings());
+            std::vector<T> input = c->get_vector<T>();


Is there a way to avoid the copy here i.e., use get_data_pointer()? If the lifecycle doesn't permit that, in that case can we use a emplace type operation so that the data is transferred to the kernel without the copy?
(This is a general questions about this pattern used in other ops as well)

…t instead. Constant is not a templated class.

ransford2011

Looks like there is support for the constructor needed by the MXNet bridge 😄 . LGTM

ransford2011 · 2017-12-19T19:49:56Z

@rkimballn1 . Just to be clear. Constant ops are only created through the factory function create() right? In @mbrookhart PR, he's expecting a string as the last parameter

rkimballn1 requested review from aprocter, diyessi, avijit-nervana and mbrookhart December 15, 2017 23:32

rkimballn1 force-pushed the bob/no-ptv branch from 53af5cd to 43885b5 Compare December 17, 2017 22:59

rkimballn1 changed the title ~~[WIP] Remove ParameterizedConstant~~ Remove ParameterizedConstant Dec 17, 2017

mbrookhart mentioned this pull request Dec 18, 2017

Refactor to match API changes in Bob's remove Parameterized PR NervanaSystems/ngraph-mxnet#104

Closed

diyessi suggested changes Dec 18, 2017

View reviewed changes

aprocter suggested changes Dec 18, 2017

View reviewed changes

aprocter mentioned this pull request Dec 18, 2017

Element Type simplification #313

Merged

avijit-nervana suggested changes Dec 18, 2017

View reviewed changes

aprocter approved these changes Dec 19, 2017

View reviewed changes

rkimballn1 force-pushed the bob/no-ptv branch from 15b0054 to ba6bc12 Compare December 19, 2017 18:34

diyessi approved these changes Dec 19, 2017

View reviewed changes

diyessi requested review from ransford2011 and removed request for mbrookhart December 19, 2017 18:59

avijit-nervana approved these changes Dec 19, 2017

View reviewed changes

rkimballn1 added 13 commits December 19, 2017 11:18

remove use of ParameterizedConstant from unit test. Now using Constan…

edb9ea1

…t instead. Constant is not a templated class.

fix merge error

1966af8

all unit tests pass

058eded

remove ParameterizedConstant unit tests

ab68743

remove ParameterizedConstant from NGVM

9654649

remove ParameterizedConstant from CPU and INTERPRETER

5634313

remove ParameterizedConstant

768135d

cleanup

a3c728a

cleanup

d9d9e23

cleanup

6d32f53

cleanup

12ac62c

move ParameterizedTensorView to NGVM directory

d8571cf

element_type cleanup

98353c9

rkimballn1 added 5 commits December 19, 2017 11:18

update per PR comment

44ee267

update per PR comment

f6e039c

update per PR comment

6c7f0e0

update per PR comment

118dda0

fix conflict

60fb9de

rkimballn1 force-pushed the bob/no-ptv branch from ba6bc12 to 60fb9de Compare December 19, 2017 19:19

ransford2011 approved these changes Dec 19, 2017

View reviewed changes

ransford2011 closed this Dec 19, 2017

ransford2011 reopened this Dec 19, 2017

rkimballn1 merged commit 833a8f1 into master Dec 19, 2017

rkimballn1 deleted the bob/no-ptv branch December 19, 2017 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ParameterizedConstant #309

Remove ParameterizedConstant #309

rkimballn1 commented Dec 15, 2017

diyessi left a comment

diyessi Dec 18, 2017

avijit-nervana Dec 18, 2017

aprocter Dec 18, 2017

diyessi Dec 18, 2017

avijit-nervana Dec 18, 2017

aprocter Dec 18, 2017

diyessi Dec 18, 2017

diyessi Dec 18, 2017

diyessi Dec 18, 2017

rkimballn1 Dec 18, 2017

diyessi Dec 18, 2017

diyessi Dec 18, 2017

aprocter left a comment

aprocter Dec 18, 2017

aprocter Dec 18, 2017

aprocter Dec 18, 2017

aprocter Dec 18, 2017

aprocter Dec 18, 2017

aprocter Dec 18, 2017

aprocter Dec 18, 2017

avijit-nervana left a comment

avijit-nervana Dec 18, 2017

aprocter Dec 18, 2017 •

edited

Loading

avijit-nervana Dec 18, 2017

avijit-nervana Dec 18, 2017

avijit-nervana Dec 18, 2017

avijit-nervana Dec 18, 2017

aprocter Dec 18, 2017 •

edited

Loading

avijit-nervana Dec 18, 2017

avijit-nervana Dec 18, 2017

avijit-nervana Dec 18, 2017

ransford2011 left a comment

ransford2011 commented Dec 19, 2017

Remove ParameterizedConstant #309

Remove ParameterizedConstant #309

Conversation

rkimballn1 commented Dec 15, 2017

diyessi 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprocter 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

avijit-nervana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprocter Dec 18, 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

aprocter Dec 18, 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

ransford2011 left a comment

Choose a reason for hiding this comment

ransford2011 commented Dec 19, 2017

aprocter Dec 18, 2017 •

edited

Loading

aprocter Dec 18, 2017 •

edited

Loading