Work-around bug in topological sort where unnused parameters aren't included in the pass

Multiply and Add numeric/autodiff unit tests pass

src/ngraph/function.hpp

@@ -40,11 +40,11 @@ namespace ngraph
                  const std::string& name = "");
 
         std::shared_ptr<Node> get_result() { return m_result; }
-        const std::vector<std::shared_ptr<op::Parameter>> get_parameters() const
+        const std::vector<std::shared_ptr<op::Parameter>>& get_parameters() const
         {
             return m_parameters;
         }
-        const std::shared_ptr<const ValueType> get_result_type() const { return m_result_type; }
+        std::shared_ptr<const ValueType> get_result_type() const { return m_result_type; }
         std::string get_name() const;
         void set_name(const std::string& name);
         std::list<std::shared_ptr<Node>>& get_ops();

src/ngraph/node.cpp

@@ -109,6 +109,8 @@ void Node::assign_tensors()
         }
         argno++;
     }
+    // Work-around until topological sort includes unnused parameters
+    m_assign_tensors_is_done = true;
 }
 
 bool Node::is_parameter() const

src/ngraph/node.hpp

@@ -116,6 +116,7 @@ namespace ngraph
 
         std::shared_ptr<Node> backprop_node(const std::shared_ptr<Node>& x,
                                             const std::shared_ptr<Node>& c);
+        bool m_assign_tensors_is_done{false};
 
     protected:
         Nodes m_arguments;

src/ngraph/ops/parameter.cpp

@@ -21,8 +21,6 @@ using namespace ngraph::op;
 
 Parameter::Parameter(const std::shared_ptr<const ValueType>& value_type)
     : Node(value_type)
-    , m_function(nullptr)
-    , m_index(0)
 {
 }
 

src/ngraph/ops/parameter.hpp

@@ -29,13 +29,7 @@ namespace ngraph
         ///
         class Parameter : public Node
         {
-            friend class ngraph::Function;
-
         protected:
-            // Called by the Function constructor to associate this parameter with the function.
-            // It is an error to try to associate a parameter with more than one function.
-            void assign_function(Function* function, size_t index);
-
             virtual void generate_adjoints(autodiff::Adjoints& adjoints,
                                            const std::shared_ptr<Node>& delta) override
             {
@@ -47,10 +41,6 @@ namespace ngraph
 
             std::string description() const override { return "Parameter"; }
             virtual void propagate_types() override;
-
-        protected:
-            Function* m_function;
-            size_t m_index;
         };
     }
 }

src/ngraph/runtime/ngvm/call_frame.cpp

@@ -39,7 +39,15 @@ void CallFrame::tensor_call(
     const std::vector<std::shared_ptr<ngraph::runtime::TensorView>>& inputs,
     const std::vector<std::shared_ptr<ngraph::runtime::TensorView>>& outputs)
 {
+    if (inputs.size() != m_n_inputs)
+    {
+        throw ngraph_error("Incorrect number of inputs");
+    }
     copy(inputs.begin(), inputs.end(), m_tensor_views.begin());
+    if (outputs.size() != m_n_outputs)
+    {
+        throw ngraph_error("Incorrect number of outputs");
+    }
     copy(outputs.begin(), outputs.end(), m_tensor_views.begin() + m_n_inputs);
     m_next_pc = m_initial_pc;
     m_return = false;

src/ngraph/runtime/ngvm/external_function.cpp

@@ -1042,6 +1042,11 @@ void ExternalFunction::compile(FunctionMap& function_map)
     // First come the function inputs
     for (auto param : m_function->get_parameters())
     {
+        // Work-around until topological sort includes unnused parameters
+        if (!param->m_assign_tensors_is_done)
+        {
+            param->assign_tensors();
+        }
         for (const descriptor::Output& output : param->get_outputs())
         {
             auto tv = output.get_tensor_view();

src/ngraph/test/random.hpp

+// ----------------------------------------------------------------------------
+// Copyright 2017 Nervana Systems Inc.
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// ----------------------------------------------------------------------------
+
+#pragma once
+
+#include <functional>
+#include <random>
+
+#include "ngraph/types/element_type.hpp"
+
+namespace ngraph
+{
+    namespace test
+    {
+        /// @brief A predictable pseudo-random number generator
+        /// The seed is initialized so that we get repeatable pseudo-random numbers for tests
+        template <typename ET>
+        class Uniform
+        {
+        public:
+            using type = typename ET::type;
+
+            Uniform(type min, type max, type seed = 0)
+                : m_engine(seed)
+                , m_distribution(min, max)
+                , m_r(std::bind(m_distribution, m_engine))
+            {
+            }
+
+            /// @brief Randomly initialize a tensor
+            /// @param ptv The tensor to initialize
+            const std::shared_ptr<runtime::ParameterizedTensorView<ET>>
+                initialize(const std::shared_ptr<runtime::ParameterizedTensorView<ET>>& ptv)
+            {
+                for (auto& elt : ptv->get_vector())
+                {
+                    elt = m_r();
+                }
+                return ptv;
+            }
+
+        protected:
+            std::default_random_engine m_engine;
+            std::uniform_real_distribution<type> m_distribution;
+            std::function<type()> m_r;
+        };
+    }
+}

test/autodiff.cpp

@@ -23,6 +23,7 @@
 #include "ngraph/autodiff/numeric_derivative.hpp"
 #include "ngraph/ngraph.hpp"
 #include "ngraph/test/all_close.hpp"
+#include "ngraph/test/random.hpp"
 
 using namespace std;
 using namespace ngraph;
@@ -39,66 +40,55 @@ TEST(backwards, parameter)
 
 TEST(backwards, add)
 {
-    auto shape = Shape{2, 3};
-    auto X0 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
-    auto X1 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
-    auto Y = X0 + X1;
-    auto C = make_shared<op::Parameter>(element::Float32::element_type(), shape);
-    auto DYDX0 = Y->backprop_node(X0, C);
-    auto DYDX1 = Y->backprop_node(X1, C);
-    ASSERT_EQ(DYDX0, C);
-    ASSERT_EQ(DYDX1, C);
-}
+    auto manager = runtime::Manager::get("NGVM");
+    auto backend = manager->allocate_backend();
 
-TEST(backwards, multiply)
-{
+    test::Uniform<element::Float32> rng(-1.0f, 1.0f);
     auto shape = Shape{2, 3};
+    auto x0 = rng.initialize(backend->make_parameterized_tensor_view<element::Float32>(shape));
+    auto x1 = rng.initialize(backend->make_parameterized_tensor_view<element::Float32>(shape));
+
     auto make_graph = [shape]() {
         auto X0 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
         auto X1 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
         return make_shared<Function>(
-            X0 * X1, nullptr, std::vector<std::shared_ptr<op::Parameter>>{X0, X1});
+            X0 + X1, nullptr, std::vector<std::shared_ptr<op::Parameter>>{X0, X1});
     };
 
+    auto results_num = autodiff::numeric_derivative<element::Float32>(
+        manager, backend, make_graph(), {x0, x1}, .001f);
+    auto results_sym =
+        autodiff::backprop_derivative<element::Float32>(manager, backend, make_graph(), {x0, x1});
+    for (size_t i = 0; i < results_num.size(); ++i)
+    {
+        auto result_num = results_num[i];
+        auto result_sym = results_sym[i];
+        bool ac = test::all_close(result_num, result_sym, .01f, .01f);
+        EXPECT_TRUE(ac);
+    }
+}
+
+TEST(backwards, multiply)
+{
     auto manager = runtime::Manager::get("NGVM");
     auto backend = manager->allocate_backend();
 
-    auto external = manager->compile(ngraph::autodiff::backprop_function(make_graph()));
-    auto cf = backend->make_call_frame(external);
-
-    auto x0 = backend->make_parameterized_tensor_view<element::Float32>(
-        runtime::NDArray<float, 2>({{1, 3, 5}, {7, 9, 11}}));
-    auto x1 = backend->make_parameterized_tensor_view<element::Float32>(
-        runtime::NDArray<float, 2>({{0, 2, 4}, {6, 8, 10}}));
-    auto c = backend->make_parameterized_tensor_view<element::Float32>(
-        runtime::NDArray<float, 2>({{0, 0, 0}, {0, 0, 0}}));
-
-    auto dx0 = backend->make_parameterized_tensor_view<element::Float32>(shape);
-    auto dx1 = backend->make_parameterized_tensor_view<element::Float32>(shape);
-    auto dx = backend->make_tuple({dx0, dx1});
+    test::Uniform<element::Float32> rng(-1.0f, 1.0f);
+    auto shape = Shape{2, 3};
+    auto x0 = rng.initialize(backend->make_parameterized_tensor_view<element::Float32>(shape));
+    auto x1 = rng.initialize(backend->make_parameterized_tensor_view<element::Float32>(shape));
 
-    size_t n = x0->get_vector().size();
-    vector<float> dx0_correct(n);
-    vector<float> dx1_correct(n);
-    for (size_t i = 0; i < n; i++)
-    {
-        c->get_vector().assign(n, 0);
-        c->get_vector()[i] = 1;
-        (*cf)({x0, x1, c}, {dx});
-        dx0_correct.assign(n, 0);
-        dx1_correct.assign(n, 0);
-        dx0_correct[i] = x1->get_vector()[i];
-        dx1_correct[i] = x0->get_vector()[i];
-        ASSERT_EQ(dx0->get_vector(), dx0_correct);
-        ASSERT_EQ(dx1->get_vector(), dx1_correct);
-    }
+    auto make_graph = [shape]() {
+        auto X0 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
+        auto X1 = make_shared<op::Parameter>(element::Float32::element_type(), shape);
+        return make_shared<Function>(
+            X0 * X1, nullptr, std::vector<std::shared_ptr<op::Parameter>>{X0, X1});
+    };
 
-    auto f_num = make_graph();
-    auto results_num =
-        autodiff::numeric_derivative<element::Float32>(manager, backend, f_num, {x0, x1}, .001f);
-    auto f_sym = make_graph();
+    auto results_num = autodiff::numeric_derivative<element::Float32>(
+        manager, backend, make_graph(), {x0, x1}, .001f);
     auto results_sym =
-        autodiff::backprop_derivative<element::Float32>(manager, backend, f_sym, {x0, x1});
+        autodiff::backprop_derivative<element::Float32>(manager, backend, make_graph(), {x0, x1});
     for (size_t i = 0; i < results_num.size(); ++i)
     {
         auto result_num = results_num[i];