Fix operator arguments #4575

Merged
merged 2 commits into from Jan 11, 2017

Projects

None yet

3 participants

@lx75249
Contributor
lx75249 commented Jan 7, 2017 edited
  1. argument "data" for Activation is missing
  2. Old operators use Symbol but new operators change to NDArray
  3. nnvm::Tuple<int> lacks a type name
  4. Pad use an argument typed with double, I'm not sure if it's intended.
src/operator/tensor/matrix_op-inl.h
@@ -17,7 +17,7 @@ namespace op {
struct ReshapeParam : public dmlc::Parameter<ReshapeParam> {
TShape target_shape;
bool keep_highest;
- nnvm::Tuple<int> shape;
+ TShape shape;
@sxjscience
sxjscience Jan 7, 2017 edited Member

The shape here should support signed values like reshape(shape=(-1, ...)). TShape is unsigned int.

@lx75249
lx75249 Jan 7, 2017 Contributor

Then I'd like to DMLC_DECLARE_TYPE_NAME for it, since this info is missing when using MXSymbolGetAtomicSymbolInfo, where do you think can I put the declartion?

@piiswrong
Member

We use NDArray for all inputs now

@lx75249
Contributor
lx75249 commented Jan 7, 2017

@piiswrong but actually the inputs are mx.sym right?

@lx75249
Contributor
lx75249 commented Jan 7, 2017

Reverted Symbol back to NDArray

src/operator/tensor/matrix_op-inl.h
+namespace dmlc {
+
+// Add a few patches to support signed Shape in dmlc/parameter.
+DMLC_DECLARE_TYPE_NAME(nnvm::Tuple<int>, "Shape(tuple)");
@sxjscience
sxjscience Jan 7, 2017 Member

Why do we need this?

@lx75249
lx75249 Jan 7, 2017 Contributor

see https://github.com/dmlc/mxnet/blob/master/include/mxnet/c_api.h#L561, this api returns symbol information such as arg_type_infos. Without declaring type name for nnvm::Tuple<int>, information of the reshape operator, which are required by language bindings, will be incomplete.

@sxjscience
sxjscience Jan 11, 2017 edited Member

I feel that it's not so good to add it in the matrix_op-inl.h since the Tuple<int> will be used in other files. May add it here https://github.com/dmlc/mxnet/blob/master/include/mxnet/tensor_blob.h#L283-L285 ?

@lx75249
lx75249 Jan 11, 2017 Contributor

Moved.

src/operator/pad-inl.h
@@ -30,7 +30,7 @@ enum PadOpOutputs { kOut };
struct PadParam : public dmlc::Parameter<PadParam> {
int mode;
- double constant_value;
+ float constant_value;
@piiswrong
piiswrong Jan 7, 2017 Member

This was intentional.

@lx75249
Contributor
lx75249 commented Jan 8, 2017

Reverted back pad argument type.

@lx75249 lx75249 Add type name for nnvm::Tuple<int>
eb47bb3
@piiswrong piiswrong merged commit 0e9f56a into dmlc:master Jan 11, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details
@rravu3 rravu3 pushed a commit to rravu3/mxnet that referenced this pull request Jan 21, 2017
@lx75249 lx75249 + Rahul Ravu Fix operator arguments (#4575)
* Add missing argument for activation

* Add type name for nnvm::Tuple<int>
bc3ae3f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment