You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/03/02 09:14:53 UTC
[GitHub] [incubator-mxnet] Alicia1529 opened a new pull request #17738: ffi
invocation: expand_dims, tril, diff, broadcast_to
Alicia1529 opened a new pull request #17738: ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738
## Description ##
as title
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
haojin2 commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386549533
##########
File path: src/api/operator/numpy/matrix_op.cc
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file matrix_op.cc
Review comment:
better rename the file to `np_matrix_op.cc` to keep the consistency.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386780651
##########
File path: src/api/operator/numpy/matrix_op.cc
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file matrix_op.cc
+ * \brief Implementation of the API of functions in src/operator/tensor/matrix_op.cc
+ */
+#include "../utils.h"
+#include "../../../operator/tensor/matrix_op-inl.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.expand_dims")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_expand_dims");
+ nnvm::NodeAttrs attrs;
+ op::ExpandDimParam param;
+ param.axis = args[1].operator int64_t();
+
+ attrs.parsed = std::move(param);
Review comment:
`ExpandDim` contains just one int. No need to std::move here.
Suggested change:
```
// we directly copy ExpandDimParam, which is trivially-copyable
attrs.parsed = param;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] Alicia1529 opened a new pull request #17738:
[Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
Alicia1529 opened a new pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738
## Description ##
as title
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] haojin2 merged pull request #17738: [Numpy] ffi
invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
haojin2 merged pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] Alicia1529 closed pull request #17738: [Numpy]
ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
Alicia1529 closed pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386780921
##########
File path: src/api/operator/numpy/np_tril_op.cc
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file np_tril_op.cc
+ * \brief Implementation of the API of functions in src/operator/numpy/np_diff.cc
+ */
+#include "../utils.h"
+#include "../../../operator/numpy/np_tril_op-inl.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.tril")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_tril");
+ nnvm::NodeAttrs attrs;
+ op::TrilParam param;
+ param.k = args[1].operator int64_t();
+
+ attrs.parsed = std::move(param);
Review comment:
Same as above. Please remove std::move and add a comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r389303091
##########
File path: src/api/operator/numpy/np_broadcast_reduce_op_value.cc
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file broadcast_reduce_op_value.cc
+ * \brief Implementation of the API of functions in
+ * src/operator/tensor/np_broadcast_reduce_op_value.cc
+ */
+#include <mxnet/api_registry.h>
+#include <mxnet/runtime/packed_func.h>
+#include "../utils.h"
+#include "../../../operator/tensor/broadcast_reduce_op.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.broadcast_to")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_broadcast_to");
+ nnvm::NodeAttrs attrs;
+ op::BroadcastToParam param;
+ if (args[1].type_code() == kDLInt) {
+ param.shape = TShape(1, args[1].operator int64_t());
+ } else {
+ param.shape = TShape(args[1].operator ObjectRef());
+ }
+ attrs.parsed = std::move(param);
+ attrs.op = op;
+ SetAttrDict<op::BroadcastToParam>(&attrs);
+
+ int num_outputs = 0;
+ NDArray* inputs[] = {args[0].operator mxnet::NDArray*()};
+ auto ndoutputs = Invoke(op, &attrs, 1, inputs, &num_outputs, nullptr);
Review comment:
The `1` seems like a magic number. Better to use `int num_inputs = 1` and call `Invoke` with `num_inputs`. Same for 3 other operators.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386781978
##########
File path: src/api/operator/numpy/np_broadcast_reduce_op_value.cc
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file broadcast_reduce_op_value.cc
+ * \brief Implementation of the API of functions in
+ * src/operator/tensor/np_broadcast_reduce_op_value.cc
+ */
+#include "../utils.h"
+#include "../../../operator/tensor/broadcast_reduce_op.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.broadcast_to")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_broadcast_to");
+ nnvm::NodeAttrs attrs;
+ op::BroadcastToParam param;
+ if (args[1].type_code() == kDLInt) {
+ param.shape = TShape(1, args[1].operator int64_t());
+ } else {
Review comment:
Please align.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386780867
##########
File path: src/api/operator/numpy/np_diff_op.cc
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file np_diff_op.cc
+ * \brief Implementation of the API of functions in src/operator/numpy/np_diff.cc
+ */
+#include "../utils.h"
+#include "../../../operator/numpy/np_diff-inl.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.diff")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_diff");
+ nnvm::NodeAttrs attrs;
+ op::DiffParam param;
+ param.n = args[1].operator int64_t();
+ param.axis = args[2].operator int64_t();
+
+ attrs.parsed = std::move(param);
Review comment:
Same as above. Please remove std::move and add a comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386777113
##########
File path: src/api/operator/numpy/np_diff_op.cc
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file np_diff_op.cc
+ * \brief Implementation of the API of functions in src/operator/numpy/np_diff.cc
+ */
+#include "../utils.h"
+#include "../../../operator/numpy/np_diff-inl.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.diff")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_diff");
+ nnvm::NodeAttrs attrs;
+ op::DiffParam param;
+ param.n = args[1].operator int64_t();
Review comment:
Why not `operator int()` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-mxnet] hzfan commented on a change in pull request
#17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
Posted by GitBox <gi...@apache.org>.
hzfan commented on a change in pull request #17738: [Numpy] ffi invocation: expand_dims, tril, diff, broadcast_to
URL: https://github.com/apache/incubator-mxnet/pull/17738#discussion_r386778159
##########
File path: src/api/operator/numpy/matrix_op.cc
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.
+ */
+
+/*!
+ * \file matrix_op.cc
+ * \brief Implementation of the API of functions in src/operator/tensor/matrix_op.cc
+ */
+#include "../utils.h"
+#include "../../../operator/tensor/matrix_op-inl.h"
+
+namespace mxnet {
+
+MXNET_REGISTER_API("_npi.expand_dims")
+.set_body([](runtime::MXNetArgs args, runtime::MXNetRetValue* ret) {
+ using namespace runtime;
+ const nnvm::Op* op = Op::Get("_npi_expand_dims");
+ nnvm::NodeAttrs attrs;
+ op::ExpandDimParam param;
+ param.axis = args[1].operator int64_t();
Review comment:
`param.axis` is int, seems that `operator int()` is more reasonable here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services