subject:"\[GitHub\] cjolivier01 commented on a change in pull request #7903\: Refactor AdaGrad optimizer to support sparse tensors"

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-13 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144683082
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op.h
 ##
 @@ -155,6 +78,15 @@ class ElemwiseBinaryOp : public OpBase {
   };
 
  private:
+  /*!
+   * \brief CSR operation requires temp space
+   */
+  struct ResourceRequest {
 
 Review comment:
   Used to index into ctx.resources during CSR pass. It is, in fact, 
referencing a RespurceRequest.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144342870
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
 
 Review comment:
   Done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144342890
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
+   dispatch_mode, DispatchMode::kFComputeEx);
+  }
+  return storage_type_assign(_attrs[0], kDefaultStorage,
+ dispatch_mode, DispatchMode::kFCompute);
+}
+
+static bool StorageTypeScatteredScalarOp(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  // Supports kDefaultStorage, kRowSparseStorage and kCSRStorage
+  const NDArrayStorageType stype = 
static_cast((*in_attrs)[0]);
+  return storage_type_assign(out_attrs,
 
 Review comment:
   Done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144341539
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -750,12 +754,39 @@ def update(self, index, weight, grad, state):
 lr = self._get_lr(index)
 wd = self._get_wd(index)
 
+is_sparse = True if weight.stype == 'row_sparse' and grad.stype == 
'row_sparse' else False
+
+if is_sparse is True:
 
 Review comment:
   ic
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144325001
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op.h
 ##
 @@ -373,21 +351,21 @@ class ElemwiseBinaryOp : public OpBase {
 CHECK_EQ(outputs.size(), 1);
 if (req[0] == kNullOp) return;
 const auto lhs_stype = inputs[0].storage_type();
-const auto rhs_stype = inputs[1].storage_type();
 const auto out_stype = outputs[0].storage_type();
 mshadow::Stream *s = ctx.get_stream();
+bool has_both = false;
 // rsp, rsp -> rsp
 
 Review comment:
   Sure. Because even though it took 2 weeks for CI to do a successful build 
without CI problems, why not make it another two weeks?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144331068
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -750,12 +754,39 @@ def update(self, index, weight, grad, state):
 lr = self._get_lr(index)
 wd = self._get_wd(index)
 
+is_sparse = True if weight.stype == 'row_sparse' and grad.stype == 
'row_sparse' else False
+
+if is_sparse is True:
 
 Review comment:
   Really? Does scipy do that as well?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144326263
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
 
 Review comment:
   You know, these all pass unit tests for every possible combination.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-12 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144325001
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op.h
 ##
 @@ -373,21 +351,21 @@ class ElemwiseBinaryOp : public OpBase {
 CHECK_EQ(outputs.size(), 1);
 if (req[0] == kNullOp) return;
 const auto lhs_stype = inputs[0].storage_type();
-const auto rhs_stype = inputs[1].storage_type();
 const auto out_stype = outputs[0].storage_type();
 mshadow::Stream *s = ctx.get_stream();
+bool has_both = false;
 // rsp, rsp -> rsp
 
 Review comment:
   Sure. Because even though it took 2 weeks for CI to do a successful build 
without CI problems, why not make it another two weeks?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144092379
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -57,7 +58,12 @@ NNVM_REGISTER_OP(_backward_add)
 .set_attr("FComputeEx",
   ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>)
 
 Review comment:
   done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144092337
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -85,7 +91,12 @@ NNVM_REGISTER_OP(_backward_sub)
   mshadow_op::identity, mshadow_op::negation>)
 .set_attr("FComputeEx", 
ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FResourceRequest",  /* For Sparse CSR */
+  [](const NodeAttrs& attrs) {
+return 
std::vector{ResourceRequest::kTempSpace};
+  })
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>);
 
 Review comment:
   ah I see hmm
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144090025
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -85,7 +91,12 @@ NNVM_REGISTER_OP(_backward_sub)
   mshadow_op::identity, mshadow_op::negation>)
 .set_attr("FComputeEx", 
ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FResourceRequest",  /* For Sparse CSR */
+  [](const NodeAttrs& attrs) {
+return 
std::vector{ResourceRequest::kTempSpace};
+  })
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>);
 
 Review comment:
   Add back will be in another PR
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144089950
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -85,7 +91,12 @@ NNVM_REGISTER_OP(_backward_sub)
   mshadow_op::identity, mshadow_op::negation>)
 .set_attr("FComputeEx", 
ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FResourceRequest",  /* For Sparse CSR */
+  [](const NodeAttrs& attrs) {
+return 
std::vector{ResourceRequest::kTempSpace};
+  })
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>);
 
 Review comment:
   the backward passes.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144082655
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -85,7 +91,12 @@ NNVM_REGISTER_OP(_backward_sub)
   mshadow_op::identity, mshadow_op::negation>)
 .set_attr("FComputeEx", 
ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FResourceRequest",  /* For Sparse CSR */
+  [](const NodeAttrs& attrs) {
+return 
std::vector{ResourceRequest::kTempSpace};
+  })
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>);
 
 Review comment:
   It appears that you removed support for maximum, minimum, etc. even though 
there are unit tests?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144082655
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cc
 ##
 @@ -85,7 +91,12 @@ NNVM_REGISTER_OP(_backward_sub)
   mshadow_op::identity, mshadow_op::negation>)
 .set_attr("FComputeEx", 
ElemwiseBinaryOp::BackwardUseNoneEx)
-.set_attr("FInferStorageType", ElemwiseStorageType<1, 2, 
true, true, false>);
+.set_attr("FResourceRequest",  /* For Sparse CSR */
+  [](const NodeAttrs& attrs) {
+return 
std::vector{ResourceRequest::kTempSpace};
+  })
+.set_attr("FInferStorageType",
+ ElemwiseStorageType<1, 2, true, true, false>);
 
 Review comment:
   It appears that you removed support for maximum, minimum, etc. even though 
there are unit tests.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144075868
 
 

 ##
 File path: src/operator/tensor/matrix_op.cc
 ##
 @@ -389,13 +389,48 @@ Example::
 
 clip(x,1,8) = [ 1.,  1.,  2.,  3.,  4.,  5.,  6.,  7.,  8.,  8.]
 
+The storage type of ``clip`` output depends on storage types of inputs and the 
a_min, a_max \
+parameter values:
+
+   - clip(default) = default
+   - clip(row_sparse, a_min <= 0, a_max >= 0) = row_sparse
+   - clip(csr, a_min <= 0, a_max >= 0) = csr
+   - clip(row_sparse, a_min < 0, a_max < 0) = default
+   - clip(row_sparse, a_min > 0, a_max > 0) = default
+   - clip(csr, a_min < 0, a_max < 0) = csr
+   - clip(csr, a_min > 0, a_max > 0) = csr
+
 )code" ADD_FILELINE)
 .set_num_inputs(1)
 .set_num_outputs(1)
 .set_attr_parser(ParamParser)
 .set_attr("FInferShape", ElemwiseShape<1, 1>)
 .set_attr("FInferType", ElemwiseType<1, 1>)
 .set_attr("FCompute", Clip)
+.set_attr("FComputeEx", ClipEx)
+.set_attr("FInferStorageType", [](const nnvm::NodeAttrs& 
attrs,
+ const int dev_mask,
+ DispatchMode* 
dispatch_mode,
+ std::vector 
*in_attrs,
+ std::vector 
*out_attrs) {
+// For clipping ranges that cross zero, sparse output is possible
+CHECK_EQ(in_attrs->size(), 1U) << " in operator " << attrs.name;
+CHECK_EQ(out_attrs->size(), 1U) << " in operator " << attrs.name;
+if ((*in_attrs)[0] == kDefaultStorage) {
+  return storage_type_assign(_attrs[0], kDefaultStorage,
 
 Review comment:
   ok
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144074539
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_scalar_op_basic.cc
 ##
 @@ -21,18 +21,105 @@
  * \file elemwise_binary_scalar_op.cc
  * \brief CPU Implementation of unary function.
  */
+#include "../../common/utils.h"
 #include "./elemwise_binary_op.h"
 #include "./elemwise_binary_scalar_op.h"
 
+#define 
MXNET_OPERATOR_REGISTER_BINARY_WITH_SCALAR_SUPPORT_WITH_DENSE_RESULT(name)\
+  NNVM_REGISTER_OP(name)\
+  .set_num_inputs(1)\
+  .set_num_outputs(1)   \
+  .set_attr_parser([](NodeAttrs* attrs) {   \
+  attrs->parsed = std::stod(attrs->dict["scalar"]); \
+})  \
+  .set_attr("FInferShape", ElemwiseShape<1, 1>)  \
+  .set_attr("FInferType", ElemwiseType<1, 1>) \
+  .set_attr("FInferStorageType", \
+BinaryScalarStorageTypeWithDenseResultStorageType)  \
+  .set_attr("FInplaceOption", \
+[](const NodeAttrs& attrs){ \
+  return std::vector >{{0, 0}}; \
+})  \
+  .add_argument("data", "NDArray-or-Symbol", "source input")\
+  .add_argument("scalar", "float", "scalar input")
+
 namespace mxnet {
 namespace op {
-MXNET_OPERATOR_REGISTER_BINARY_SCALAR(_plus_scalar)
+
+static bool BinaryScalarStorageTypeWithDenseResultStorageType(const NodeAttrs& 
attrs,
+  const int 
dev_mask,
+  DispatchMode* 
dispatch_mode,
+  
std::vector* in_attrs,
+  
std::vector* out_attrs)  {
+  if (common::ContainsOnlyStorage(*in_attrs, kDefaultStorage)) {
 
 Review comment:
   ok...
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144073958
 
 

 ##
 File path: src/operator/tensor/matrix_op.cc
 ##
 @@ -389,13 +389,48 @@ Example::
 
 clip(x,1,8) = [ 1.,  1.,  2.,  3.,  4.,  5.,  6.,  7.,  8.,  8.]
 
+The storage type of ``clip`` output depends on storage types of inputs and the 
a_min, a_max \
+parameter values:
+
+   - clip(default) = default
+   - clip(row_sparse, a_min <= 0, a_max >= 0) = row_sparse
+   - clip(csr, a_min <= 0, a_max >= 0) = csr
+   - clip(row_sparse, a_min < 0, a_max < 0) = default
+   - clip(row_sparse, a_min > 0, a_max > 0) = default
+   - clip(csr, a_min < 0, a_max < 0) = csr
+   - clip(csr, a_min > 0, a_max > 0) = csr
+
 )code" ADD_FILELINE)
 .set_num_inputs(1)
 .set_num_outputs(1)
 .set_attr_parser(ParamParser)
 .set_attr("FInferShape", ElemwiseShape<1, 1>)
 .set_attr("FInferType", ElemwiseType<1, 1>)
 .set_attr("FCompute", Clip)
+.set_attr("FComputeEx", ClipEx)
+.set_attr("FInferStorageType", [](const nnvm::NodeAttrs& 
attrs,
+ const int dev_mask,
+ DispatchMode* 
dispatch_mode,
+ std::vector 
*in_attrs,
+ std::vector 
*out_attrs) {
+// For clipping ranges that cross zero, sparse output is possible
+CHECK_EQ(in_attrs->size(), 1U) << " in operator " << attrs.name;
+CHECK_EQ(out_attrs->size(), 1U) << " in operator " << attrs.name;
+if ((*in_attrs)[0] == kDefaultStorage) {
+  return storage_type_assign(_attrs[0], kDefaultStorage,
 
 Review comment:
   Why not?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-11 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r144073958
 
 

 ##
 File path: src/operator/tensor/matrix_op.cc
 ##
 @@ -389,13 +389,48 @@ Example::
 
 clip(x,1,8) = [ 1.,  1.,  2.,  3.,  4.,  5.,  6.,  7.,  8.,  8.]
 
+The storage type of ``clip`` output depends on storage types of inputs and the 
a_min, a_max \
+parameter values:
+
+   - clip(default) = default
+   - clip(row_sparse, a_min <= 0, a_max >= 0) = row_sparse
+   - clip(csr, a_min <= 0, a_max >= 0) = csr
+   - clip(row_sparse, a_min < 0, a_max < 0) = default
+   - clip(row_sparse, a_min > 0, a_max > 0) = default
+   - clip(csr, a_min < 0, a_max < 0) = csr
+   - clip(csr, a_min > 0, a_max > 0) = csr
+
 )code" ADD_FILELINE)
 .set_num_inputs(1)
 .set_num_outputs(1)
 .set_attr_parser(ParamParser)
 .set_attr("FInferShape", ElemwiseShape<1, 1>)
 .set_attr("FInferType", ElemwiseType<1, 1>)
 .set_attr("FCompute", Clip)
+.set_attr("FComputeEx", ClipEx)
+.set_attr("FInferStorageType", [](const nnvm::NodeAttrs& 
attrs,
+ const int dev_mask,
+ DispatchMode* 
dispatch_mode,
+ std::vector 
*in_attrs,
+ std::vector 
*out_attrs) {
+// For clipping ranges that cross zero, sparse output is possible
+CHECK_EQ(in_attrs->size(), 1U) << " in operator " << attrs.name;
+CHECK_EQ(out_attrs->size(), 1U) << " in operator " << attrs.name;
+if ((*in_attrs)[0] == kDefaultStorage) {
+  return storage_type_assign(_attrs[0], kDefaultStorage,
 
 Review comment:
   Why not?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143848982
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
+   dispatch_mode, DispatchMode::kFComputeEx);
+  }
+  return storage_type_assign(_attrs[0], kDefaultStorage,
+ dispatch_mode, DispatchMode::kFCompute);
+}
+
+static bool StorageTypeScatteredScalarOp(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  // Supports kDefaultStorage, kRowSparseStorage and kCSRStorage
+  const NDArrayStorageType stype = 
static_cast((*in_attrs)[0]);
+  return storage_type_assign(out_attrs,
+ stype,
+ dispatch_mode,
+ stype == kDefaultStorage ? DispatchMode::kFCompute
+  : 
DispatchMode::kFComputeEx);
+}
+
+/*! \brief _scatter_elemwise_div */
+MXNET_OPERATOR_REGISTER_BINARY(_scatter_elemwise_div)
 
 Review comment:
   For example:  
https://www.tensorflow.org/versions/r1.2/api_docs/python/tf/scatter_div
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143836331
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
+   dispatch_mode, DispatchMode::kFComputeEx);
+  }
+  return storage_type_assign(_attrs[0], kDefaultStorage,
+ dispatch_mode, DispatchMode::kFCompute);
+}
+
+static bool StorageTypeScatteredScalarOp(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  // Supports kDefaultStorage, kRowSparseStorage and kCSRStorage
+  const NDArrayStorageType stype = 
static_cast((*in_attrs)[0]);
+  return storage_type_assign(out_attrs,
+ stype,
+ dispatch_mode,
+ stype == kDefaultStorage ? DispatchMode::kFCompute
+  : 
DispatchMode::kFComputeEx);
+}
+
+/*! \brief _scatter_elemwise_div */
+MXNET_OPERATOR_REGISTER_BINARY(_scatter_elemwise_div)
 
 Review comment:
   This is significantly more efficient than doing a retain first, since 
RspRspOp can create scatterred output during the calculation without the copy 
and memory allocation required to do the retain beforehand.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143832920
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
+   dispatch_mode, DispatchMode::kFComputeEx);
+  }
+  return storage_type_assign(_attrs[0], kDefaultStorage,
+ dispatch_mode, DispatchMode::kFCompute);
+}
+
+static bool StorageTypeScatteredScalarOp(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  // Supports kDefaultStorage, kRowSparseStorage and kCSRStorage
+  const NDArrayStorageType stype = 
static_cast((*in_attrs)[0]);
+  return storage_type_assign(out_attrs,
+ stype,
+ dispatch_mode,
+ stype == kDefaultStorage ? DispatchMode::kFCompute
+  : 
DispatchMode::kFComputeEx);
+}
+
+/*! \brief _scatter_elemwise_div */
+MXNET_OPERATOR_REGISTER_BINARY(_scatter_elemwise_div)
 
 Review comment:
   Tensorflow has similar scatter operators, although they're a bit more robust
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143832920
 
 

 ##
 File path: src/operator/tensor/elemwise_scatter_op.cc
 ##
 @@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 limitations
+ * under the License.
+ */
+#include "./elemwise_binary_op-inl.h"
+#include "./elemwise_binary_scalar_op.h"
+#include "./elemwise_scatter_op.h"
+
+namespace mxnet {
+namespace op {
+
+static bool StorageTypeRspOrDenseOutput(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  CHECK_EQ(in_attrs->size(), 2U);
+  CHECK_EQ(out_attrs->size(), 1U);
+  const NDArrayStorageType lhs_stype = 
static_cast((*in_attrs)[0]);
+  if (lhs_stype == kRowSparseStorage) {
+return storage_type_assign(_attrs[0], kRowSparseStorage,
+   dispatch_mode, DispatchMode::kFComputeEx);
+  }
+  return storage_type_assign(_attrs[0], kDefaultStorage,
+ dispatch_mode, DispatchMode::kFCompute);
+}
+
+static bool StorageTypeScatteredScalarOp(const NodeAttrs& attrs,
+const int dev_mask,
+DispatchMode* dispatch_mode,
+std::vector* in_attrs,
+std::vector* out_attrs) {
+  // Supports kDefaultStorage, kRowSparseStorage and kCSRStorage
+  const NDArrayStorageType stype = 
static_cast((*in_attrs)[0]);
+  return storage_type_assign(out_attrs,
+ stype,
+ dispatch_mode,
+ stype == kDefaultStorage ? DispatchMode::kFCompute
+  : 
DispatchMode::kFComputeEx);
+}
+
+/*! \brief _scatter_elemwise_div */
+MXNET_OPERATOR_REGISTER_BINARY(_scatter_elemwise_div)
 
 Review comment:
   Tensorflow has similar scatter operators
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143831793
 
 

 ##
 File path: src/operator/tensor/cast_storage-inl.cuh
 ##
 @@ -28,7 +28,7 @@
 #include 
 #include 
 #include 
-#include 
+#include 
 
 Review comment:
   ok...
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors + unary and binary refactoring for new infer storage type logic

2017-10-10 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors + unary and binary refactoring for new 
infer storage type logic
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r143831685
 
 

 ##
 File path: src/common/utils.h
 ##
 @@ -63,6 +63,36 @@ inline bool ContainsOnlyStorage(const StorageTypeVector& 
vstorage,
   return false;
 }
 
+/*! \brief returns true if all storage types in `vstorage` are the same as 
target `stype1`
+ * or `stype2'. Sets boolean if both found.
+ * false is returned for empty inputs.
+ */
+inline bool ContainsOnlyStorage(const StorageTypeVector& vstorage,
 
 Review comment:
   The new design may check a lot of permutations of inputs/outputs in order to 
make the decision to fall back or not.  This is the foundation of the new infer 
storage type design which is already in master.  Not sure what the expectation 
is in that case.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-20 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r140057752
 
 

 ##
 File path: tests/python/unittest/test_sparse_operator.py
 ##
 @@ -697,6 +697,19 @@ def check_binary_op_with_scalar(stype,
force_overlap=force_overlap,
verbose=False)
 
+# plus_scalar
 
 Review comment:
   Also scatter_minus
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-20 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r140057718
 
 

 ##
 File path: tests/python/unittest/test_sparse_operator.py
 ##
 @@ -697,6 +697,19 @@ def check_binary_op_with_scalar(stype,
force_overlap=force_overlap,
verbose=False)
 
+# plus_scalar
 
 Review comment:
   Yes
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-20 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r140046020
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -664,27 +667,45 @@ class AdaGrad(Optimizer):
 --
 eps: float, optional
 Small value to avoid division by 0.
+
 """
 def __init__(self, eps=1e-7, **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=weight.stype)  # 
history
 
 def update(self, index, weight, grad, state):
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
+
+if is_sparse:
+history[:] = op.elemwise_add(history, op.square(grad))
+assert history.stype == save_history_stype
+srt = op.sqrt(_internal._scatter_plus_scalar(history, 
self.float_stable_eps))
 
 Review comment:
   ok...
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-19 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139765735
 
 

 ##
 File path: tests/python/unittest/test_module.py
 ##
 @@ -462,74 +462,105 @@ def test_shared_exec_group(exec_grp_shared, 
exec_grp_created, shared_arg_names=N
 
 def test_factorization_machine_module():
 """ Test factorization machine model with sparse operators """
-mx.random.seed(11)
-rnd.seed(11)
-
-def fm(factor_size, feature_dim, init):
-x = mx.symbol.Variable("data", stype='csr')
-v = mx.symbol.Variable("v", shape=(feature_dim, factor_size),
-   init=init, stype='row_sparse')
-
-w1_weight = mx.symbol.var('w1_weight', shape=(feature_dim, 1),
-  init=init, stype='row_sparse')
-w1_bias = mx.symbol.var('w1_bias', shape=(1))
-w1 = mx.symbol.broadcast_add(mx.symbol.dot(x, w1_weight), w1_bias)
-
-v_s = mx.symbol._internal._square_sum(data=v, axis=1, keepdims=True)
-x_s = mx.symbol.square(data=x)
-bd_sum = mx.sym.dot(x_s, v_s)
-
-w2 = mx.symbol.dot(x, v)
-w2_squared = 0.5 * mx.symbol.square(data=w2)
-
-w_all = mx.symbol.Concat(w1, w2_squared, dim=1)
-sum1 = mx.symbol.sum(data=w_all, axis=1, keepdims=True)
-sum2 = 0.5 * mx.symbol.negative(bd_sum)
-model = mx.sym.elemwise_add(sum1, sum2)
-
-y = mx.symbol.Variable("label")
-model = mx.symbol.LinearRegressionOutput(data=model, label=y)
-return model
-
-# model
-ctx = default_context()
-init = mx.initializer.Normal(sigma=0.01)
-factor_size = 4
-feature_dim = 1
-model = fm(factor_size, feature_dim, init)
-
-# data iter
-num_batches = 5
-batch_size = 64
-num_samples = batch_size * num_batches
-# generate some random csr data
-csr_nd = rand_ndarray((num_samples, feature_dim), 'csr', 0.1)
-label = mx.nd.ones((num_samples,1))
-# the alternative is to use LibSVMIter
-train_iter = mx.io.NDArrayIter(data=csr_nd, label={'label':label},
-   batch_size=batch_size, 
last_batch_handle='discard')
-# create module
-mod = mx.mod.Module(symbol=model, data_names=['data'], 
label_names=['label'])
-# allocate memory by given the input data and lable shapes
-mod.bind(data_shapes=train_iter.provide_data, 
label_shapes=train_iter.provide_label)
-# initialize parameters by uniform random numbers
-mod.init_params(initializer=init)
-# use Sparse SGD with learning rate 0.1 to train
-adam = mx.optimizer.Adam(clip_gradient=5.0, learning_rate=0.001, 
rescale_grad=1.0/batch_size)
-mod.init_optimizer(optimizer=adam)
-# use accuracy as the metric
-metric = mx.metric.create('MSE')
-# train 10 epoch
-for epoch in range(10):
-train_iter.reset()
-metric.reset()
-for batch in train_iter:
-mod.forward(batch, is_train=True)   # compute predictions
-mod.update_metric(metric, batch.label)  # accumulate prediction 
accuracy
-mod.backward()  # compute gradients
-mod.update()# update parameters
-# print('Epoch %d, Training %s' % (epoch, metric.get()))
-assert(metric.get()[1] < 0.05), metric.get()[1]
+def check_factorization_machine_module(optimizer=None, num_epochs=None):
 
 Review comment:
   test_optimizer appears to test C++ version against python version. There is 
only a python version for AdaGrad, therefore it's not clear what it tests 
against.  I am using the test_module() test with an expected accuracy rate to 
test.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-19 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139756939
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 def update(self, index, weight, grad, state):
+#print("ENTER ADAGRAD UPDATE")
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
 
 Review comment:
   many of these ops support both sparse and dense input combinations (and 
handle them in an efficient manner without fallback). not to say it's the most 
efficient way to do it, but it's legal.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-19 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139756939
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 def update(self, index, weight, grad, state):
+#print("ENTER ADAGRAD UPDATE")
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
 
 Review comment:
   many of these ops support both sparse and dense input combinations. not to 
say it's the most efficient way to do it, but it's legal.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-19 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139724976
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -670,21 +672,37 @@ def __init__(self, eps=1e-7, **kwargs):
 self.float_stable_eps = eps
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=weight.stype)  # 
history
 
 def update(self, index, weight, grad, state):
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
+
+if is_sparse:
+history[:] = op.elemwise_add(history, op.square(grad))
+assert history.stype == save_history_stype
+srt = op.sqrt(_internal._scatter_plus_scalar(history, 
self.float_stable_eps))
+assert srt.stype == save_history_stype
+div = _internal._scatter_elemwise_div(grad, srt)
+assert div.stype == grad.stype
+else:
+history[:] += square(grad)
+div = grad / sqrt(history + self.float_stable_eps)
+
+weight[:] += (div + weight * wd) * -lr
 
 Review comment:
   Ah, that version made sense! thanks!
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-18 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139483314
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 
 Review comment:
   done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-18 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139483293
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 Review comment:
   done
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-18 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139461575
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 def update(self, index, weight, grad, state):
+#print("ENTER ADAGRAD UPDATE")
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
+
+if is_sparse:
+history[:] = op.elemwise_add(history, op.square(grad))
+assert history.stype == save_history_stype
+srt = op.sqrt(history)
 
 Review comment:
   ok, will scatter_plus them
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-18 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139461575
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 def update(self, index, weight, grad, state):
+#print("ENTER ADAGRAD UPDATE")
 assert(isinstance(weight, NDArray))
 assert(isinstance(grad, NDArray))
 self._update_count(index)
 lr = self._get_lr(index)
 wd = self._get_wd(index)
-
+save_grad_stype = grad.stype
 grad = grad * self.rescale_grad
 if self.clip_gradient is not None:
 grad = clip(grad, -self.clip_gradient, self.clip_gradient)
 history = state
-history[:] += (grad * grad)
-weight[:] += -lr * (grad / sqrt(history + self.float_stable_eps) + wd 
* weight)
+save_history_stype = history.stype
+
+is_sparse = True if weight.stype != 'default' or grad.stype != 
'default' else False
+
+if is_sparse:
+history[:] = op.elemwise_add(history, op.square(grad))
+assert history.stype == save_history_stype
+srt = op.sqrt(history)
 
 Review comment:
   ok
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-15 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139268501
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 super(AdaGrad, self).__init__(**kwargs)
 self.float_stable_eps = eps
+self.stype = stype
 
 def create_state(self, index, weight):
-return zeros(weight.shape, weight.context)  # history
+return zeros(weight.shape, weight.context, stype=self.stype)  # history
 
 Review comment:
   they don't, actually. I get update calls for sized-1 dense weights along 
with the sparse ones. is the update not expected to occur then? Because the 
non-sparse version updates them.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-15 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139268189
 
 

 ##
 File path: python/mxnet/optimizer.py
 ##
 @@ -665,26 +667,46 @@ class AdaGrad(Optimizer):
 eps: float, optional
 Small value to avoid division by 0.
 """
-def __init__(self, eps=1e-7, **kwargs):
+def __init__(self, eps=1e-7, stype='default', **kwargs):
 
 Review comment:
   ?
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad optimizer to support sparse tensors

2017-09-15 Thread git

cjolivier01 commented on a change in pull request #7903: Refactor AdaGrad 
optimizer to support sparse tensors
URL: https://github.com/apache/incubator-mxnet/pull/7903#discussion_r139267799
 
 

 ##
 File path: src/operator/tensor/elemwise_binary_op_basic.cu
 ##
 @@ -36,21 +36,21 @@ NNVM_REGISTER_OP(_backward_add)
   ElemwiseBinaryOp::BackwardUseNoneWithHalf2);
 
-NNVM_REGISTER_OP(_sub)
+NNVM_REGISTER_OP(elemwise_sub)
 
 Review comment:
   not that I know of. I don't think anyone using cpp-package uses these sort 
of operators.
   The naming inconsistency between elemwise_sub and the other similar three is 
nonsensical.
 

This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

38 matches

Mail list logo