Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-382] Shape and Size Operator #10889

Merged
merged 31 commits into from
Jun 30, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
4635957
Shape Operator
May 10, 2018
38c15d9
cuda
May 10, 2018
6540678
size op
May 11, 2018
01d1b95
lint issues
May 11, 2018
b513dbe
docs example
May 11, 2018
125c3cd
add docs, change op name to avoid conflict, add convenience confluent
May 11, 2018
ac96ef1
change name to _nd
May 11, 2018
93ffddc
fix test cases, add new kernel
May 15, 2018
3d578d3
test name fix.
May 15, 2018
ef43d2f
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
May 15, 2018
1b7ba47
solve gpu memory problem for size and shape
Jun 11, 2018
f8cc278
Merge pull request #3 from haojin2/shape_op
anirudhacharya Jun 11, 2018
eb74750
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Jun 12, 2018
08346da
get rid of FIgnoreInputs attr of shape_nd
Jun 12, 2018
d2f7999
Merge pull request #4 from haojin2/shape_op
anirudhacharya Jun 12, 2018
93fc294
Merge branch 'shape' of https://github.com/anirudhacharya/incubator-m…
Jun 13, 2018
3f84b1a
op name change
Jun 13, 2018
3d8f560
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Jun 13, 2018
2074b46
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Jun 18, 2018
4b1164e
fix
Jun 22, 2018
ee97196
Merge branch 'master' of https://github.com/apache/incubator-mxnet into
Jun 25, 2018
10cb562
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Jun 26, 2018
cbec1e5
retrigger CI
Jun 26, 2018
039e6d4
retrigger CI
Jun 26, 2018
ee110fd
retrigger CI
Jun 26, 2018
460c315
Merge branch 'master' of https://github.com/apache/incubator-mxnet in…
Jun 26, 2018
67cbbfe
trigger CI
Jun 26, 2018
804dfb4
fix comments
Jun 28, 2018
f17f9c8
cpplint
Jun 28, 2018
c188404
nit
Jun 29, 2018
de64b97
trigger CI
Jun 29, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/operator/tensor/elemwise_unary_op.h
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,26 @@ void CastCompute(const nnvm::NodeAttrs& attrs,
});
}

template<typename xpu>
void ShapeCompute(const nnvm::NodeAttrs& attrs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to make templates for shape and size functions based on the type of device. CPU and GPU FCompute functions are defined respectively in .cc and .cu and don't share anything.

const OpContext& ctx,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alignment.

const std::vector<TBlob>& inputs,
const std::vector<OpReqType>& req,
const std::vector<TBlob>& outputs) {
CHECK_EQ(inputs.size(), 1U);
CHECK_EQ(outputs.size(), 1U);
CHECK_EQ(req.size(), 1U);
const TBlob& in_data = inputs[0];
const TBlob& out_data = outputs[0];
mshadow::Stream<xpu> *s = ctx.get_stream<xpu>();
TShape in_shape = in_data.shape_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const TShape&.

MSHADOW_TYPE_SWITCH(out_data.type_flag_, DType, {
mxnet_op::Kernel<mshadow_op::identity_with_cast, xpu>::Launch(
s, in_data.ndim(), out_data.dptr<DType>(), in_shape.data());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why DType for out_data? Isn't that int64 as in the description?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i will make this change.

});
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please get rid of one blank line here, c++ use only 1 blank line between functions

struct HardSigmoidParam : public dmlc::Parameter<HardSigmoidParam> {
real_t alpha;
real_t beta;
Expand Down
31 changes: 31 additions & 0 deletions src/operator/tensor/elemwise_unary_op_basic.cc
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,37 @@ NNVM_REGISTER_OP(reshape_like)
.add_argument("lhs", "NDArray-or-Symbol", "First input.")
.add_argument("rhs", "NDArray-or-Symbol", "Second input.");

NNVM_REGISTER_OP(shape)
.describe("Returns a 1D int64 array containing the shape of data.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be clearer if you add an example here?

.set_num_inputs(1)
.set_num_outputs(1)
.set_attr<nnvm::FInplaceIdentity>("FInplaceIdentity",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will remove it.

[](const NodeAttrs& attrs){ return std::vector<bool>{true}; })
.set_attr<nnvm::FIgnoreInputs>("FIgnoreInputs",
[](const NodeAttrs& attrs) { return std::vector<uint32_t>(1, 1); })
.set_attr<FCompute>("FCompute<cpu>", ShapeCompute<cpu>)
.set_attr<nnvm::FInferShape>("FInferShape",
[](const nnvm::NodeAttrs& attrs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use spaces instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the CI failed due to cpplint. I will fix it.

std::vector<TShape> *in_attrs,
std::vector<TShape> *out_attrs) {
CHECK_EQ(in_attrs->size(), 1U);
CHECK_EQ(out_attrs->size(), 1U);
TShape target_shape(1);
target_shape[0] = in_attrs->at(0).ndim();
SHAPE_ASSIGN_CHECK(*out_attrs, 0, target_shape);
return out_attrs->at(0).ndim() != 0U && out_attrs->at(0).Size() != 0U;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use shape_is_none(out_attrs->at(0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you suggest that I define a shape_is_none function and use it here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already available.

})
.set_attr<nnvm::FInferType>("FInferType",
[](const nnvm::NodeAttrs& attrs,
std::vector<int>* in_attrs,
std::vector<int>* out_attrs) {
CHECK_EQ(in_attrs->size(), 1U);
CHECK_EQ(out_attrs->size(), 1U);
TYPE_ASSIGN_CHECK(*out_attrs, 0, 0U);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use type enum variable name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, i will use mshadow::kInt64

return out_attrs->at(0) != -1;
})
.set_attr<nnvm::FGradient>("FGradient", MakeZeroGradNodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there be no appropriate gradient definition for this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, shape and size operator will not have gradients right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'm asking the same thing. Why did you define FGradient attribute for the op?

.add_argument("data", "NDArray-or-Symbol", "Input Array.");

DMLC_REGISTER_PARAMETER(CastParam);
NNVM_REGISTER_OP(Cast)
Expand Down
3 changes: 3 additions & 0 deletions src/operator/tensor/elemwise_unary_op_basic.cu
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ NNVM_REGISTER_OP(_identity_with_attr_like_rhs)
NNVM_REGISTER_OP(reshape_like)
.set_attr<FCompute>("FCompute<gpu>", UnaryOp::IdentityCompute<gpu>);

NNVM_REGISTER_OP(shape)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the name come from? This looks confusing and conflicts with the property name shape of NDArray in Python. Need to ensure the documentation can be rendered correctly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you suggest the name of the operator should be? This is more or less the name used in a couple of other frameworks too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure the doc page can be rendered correctly at least.

.set_attr<FCompute>("FCompute<gpu>", ShapeCompute<gpu>);

NNVM_REGISTER_OP(Cast)
.set_attr<FCompute>("FCompute<gpu>", CastCompute<gpu>);

Expand Down
9 changes: 9 additions & 0 deletions tests/python/unittest/test_operator.py
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,15 @@ def fsigmoid(a):
check_symbolic_forward(y, [xa], [ya])
check_symbolic_backward(y, [xa], [np.ones(shape)], [ya * (1 - ya)])

@with_seed()
def test_shape():
shape = [3, 4]
x = mx.symbol.Variable("x")
y = mx.sym.shape(x)
xa = np.random.uniform(low=-1.0,high=1.0,size=shape)
ya = np.shape(xa)
check_symbolic_forward(y, [xa], [ya])

@with_seed()
def test_hard_sigmoid():
def fhardsigmoid(a, alpha=0.2, beta=0.5):
Expand Down