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/01/02 22:02:21 UTC
[GitHub] [incubator-mxnet] samskalicky opened a new pull request #17204:
[WIP] enhancements for MXTensor for custom operators
samskalicky opened a new pull request #17204: [WIP] enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204
## Description ##
Enhancements to MXTensor for custom operators. Adds the following features:
- version IDs to uniquely identify when a tensor has been modified or is unchanged between calls to operator (can save on data transfer to accelerator if tensor is unchanged)
- identifies tensor as auxiliary or argument to optimize data movement for accelerators
- function to compare MXTensors like we have for MXNet NDarray isSame API
## Checklist ##
### Essentials ###
Please feel free to remove inapplicable items for your PR.
- [ ] The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant [JIRA issue](https://issues.apache.org/jira/projects/MXNET/issues) created (except PRs with tiny changes)
- [ ] Changes are complete (i.e. I finished coding on this PR)
- [ ] All changes have test coverage:
- Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
- Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
- Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
- [ ] Code is well-documented:
- For user-facing API changes, API doc string has been updated.
- For new C++ functions in header files, their functionalities and arguments are documented.
- For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
- Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
- [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
----------------------------------------------------------------
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] samskalicky commented on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-571775761
@mxnet-label-bot update [pr-awaiting-merge]
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061195
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
For C++ vector container, `operator==` compares the values.
```c++
#include <iostream>
#include <vector>
using namespace std;
int main() {
vector<int> a{1,2,3};
vector<int> b{1,2,3};
vector<int> c{1,2,4};
cout << (a == b) << endl; // 1
cout << (a == c) << endl; // 0
return 0;
}
```
Although `（a==b)` is 1, `a.data()` is not equal to `b.data()`.
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363008407
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
writing a response together with @rondogency:
For clarity, here were checking the following items: dtype, version ID, shape, and the pointer address of the data in memory. If two tensors have the same data pointer then they will have the same data values also. So here the two tensors will have the same content and will have the same tensor attributes (shape, type, version ID) also.
I dont think we need to be consistent with NDarray API here, since users writing custom ops wont necessarily be familiar with NDarray API. And here we're trying to make the right API for this use-case (custom operators). Our comparison API also checks the version number, and the NDarray API does not. So we're already diverging from being consistent with NDArray API.
For the record, here is the similar NDarray API:
https://github.com/apache/incubator-mxnet/blob/55e222b8c97f99193f832f785cf210f876632add/include/mxnet/ndarray.h#L212-L217
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363005646
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
It will be more consistent with MXNet NDArray API to use ‘IsSame’, since operator== is confusing between same object and same tensor content.
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362946497
##########
File path: include/mxnet/lib_api.h
##########
@@ -209,10 +210,15 @@ enum MXReturnValue {
* \brief Tensor data structure used by custom operator
*/
struct MXTensor {
- MXTensor() : data_ptr(NULL) {}
+MXTensor() : data_ptr(NULL), dtype(kUNSET), version(0) {}
- MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype)
- : data_ptr(data_ptr), shape(shape), dtype(dtype) {}
+ MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype,
+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}
+
+ void update(void *dptr, MXDType type, size_t ver) {
Review comment:
per our discussion, lets move the the for loop to copy shape and call to setDLTensor inside this function. Change name to "setTensor"
----------------------------------------------------------------
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] wkcn merged pull request #17204: Enhancements for
MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn merged pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204
----------------------------------------------------------------
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] rondogency commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
rondogency commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362943436
##########
File path: include/mxnet/lib_api.h
##########
@@ -209,10 +210,15 @@ enum MXReturnValue {
* \brief Tensor data structure used by custom operator
*/
struct MXTensor {
- MXTensor() : data_ptr(NULL) {}
+MXTensor() : data_ptr(NULL), dtype(kUNSET), version(0) {}
- MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype)
- : data_ptr(data_ptr), shape(shape), dtype(dtype) {}
+ MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype,
+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}
Review comment:
it will be better to unify the naming across all places, like using verID in lib_api.h and 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
[GitHub] [incubator-mxnet] samskalicky commented on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-571913246
@wkcn will you merge this?
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061982
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
Thanks @wkcn, if @rondogency agrees ill change it back to 'isSame'
----------------------------------------------------------------
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] samskalicky commented on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-571775883
@wkcn we're ready to merge!
----------------------------------------------------------------
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] rondogency commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
rondogency commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362942737
##########
File path: include/mxnet/lib_api.h
##########
@@ -209,10 +210,15 @@ enum MXReturnValue {
* \brief Tensor data structure used by custom operator
*/
struct MXTensor {
- MXTensor() : data_ptr(NULL) {}
+MXTensor() : data_ptr(NULL), dtype(kUNSET), version(0) {}
- MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype)
- : data_ptr(data_ptr), shape(shape), dtype(dtype) {}
+ MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype,
+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}
+
+ void update(void *dptr, MXDType type, size_t ver) {
Review comment:
do we really need this function? it doesn't have any checks, only copy pointers. I think we can copy them line by line in lib_api.h and keep MXTensor as simple as possible
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362954687
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
done
----------------------------------------------------------------
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] rondogency commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
rondogency commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363010147
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
comparing object is how c++ is doing for vector and usually for struct, and in NDArray we don't have operators !=, <, > either, so I don't think it is going to be confusing
----------------------------------------------------------------
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] samskalicky commented on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-570619946
@rondogency @mseth10 @wkcn for review
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061195
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
For C++ vector container, `operator==` compares the values.
```c++
#include <iostream>
#include <vector>
using namespace std;
int main() {
vector<int> a{1,2,3};
vector<int> b{1,2,3};
vector<int> c{1,2,4};
cout << (a == b) << endl; // 1
cout << (a == c) << endl; // 0
cout << (a.data() == b.data()) << endl; // 0
return 0;
}
```
Although `（a==b)` is 1, `a.data()` is not equal to `b.data()`.
----------------------------------------------------------------
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] rondogency commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
rondogency commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363440140
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
ok I agree with changing it back to isSame, thanks for the inspection on c++ vector!
----------------------------------------------------------------
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] mseth10 commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
mseth10 commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363216894
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
I agree with @wkcn on this. `isSame` is less confusing.
----------------------------------------------------------------
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] wkcn commented on issue #17204: Enhancements for
MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-571981967
Merged : ) Thank you!
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363006670
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
I think operator== is confusing. For a tensor object, == usually means value comparison.
In the future, we may add other operators !=, <, >, etc.
It may be better and more consistent with MXNet NDArray API to use ‘isSame’.
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363006251
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
Sorry that I did not read the previous review.
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061583
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
Thank @rondogency and you for the explanation in detail!
1. If two tensors have the same data pointer then they will have the same data values also.
It is right. However, if two tensors have the same data values, they may not have the same data pointer. I did a simple test on C++ vector.
```c++
#include <iostream>
#include <vector>
using namespace std;
int main() {
vector<int> a{1,2,3};
vector<int> b{1,2,3};
cout << (a == b) << endl; // 1
cout << (a.data() == b.data()) << endl; // 0
return 0;
}
```
2. we don't need to be consistent with NDarray API here.
Agree : )
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061195
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
For C++ vector container, `operator==` compares the values.
```c++
#include <iostream>
#include <vector>
using namespace std;
int main() {
vector<int> a{1,2,3};
vector<int> b{1,2,3};
vector<int> c{1,2,4};
cout << (a == b) << endl; // 1
cout << (a == c) << endl; // 0
return 0;
}
```
----------------------------------------------------------------
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] rondogency commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
rondogency commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362943802
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
should we override operator==? since we won't support C anyway
----------------------------------------------------------------
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] samskalicky commented on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-570619700
@mxnet-label-bot add [pr-awaiting-review]
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r362954715
##########
File path: include/mxnet/lib_api.h
##########
@@ -209,10 +210,15 @@ enum MXReturnValue {
* \brief Tensor data structure used by custom operator
*/
struct MXTensor {
- MXTensor() : data_ptr(NULL) {}
+MXTensor() : data_ptr(NULL), dtype(kUNSET), version(0) {}
- MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype)
- : data_ptr(data_ptr), shape(shape), dtype(dtype) {}
+ MXTensor(void *data_ptr, const std::vector<int64_t> &shape, MXDType dtype,
+ size_t ID)
+ : data_ptr(data_ptr), shape(shape), dtype(dtype), version(ID) {}
Review comment:
done
----------------------------------------------------------------
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] wkcn commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
wkcn commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363061583
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +289,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool operator==(const MXTensor &oth) {
Review comment:
Thank you for the explanation in detail!
1. If two tensors have the same data pointer then they will have the same data values also.
It is right. However, if two tensors have the same data values, they may not have the same data pointer. I did a simple test on C++ vector.
```c++
#include <iostream>
#include <vector>
using namespace std;
int main() {
vector<int> a{1,2,3};
vector<int> b{1,2,3};
cout << (a == b) << endl; // 1
cout << (a.data() == b.data()) << endl; // 0
return 0;
}
```
2. we don't need to be consistent with NDarray API here.
Agree : )
----------------------------------------------------------------
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] samskalicky commented on a change in pull request
#17204: Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky commented on a change in pull request #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#discussion_r363019493
##########
File path: include/mxnet/lib_api.h
##########
@@ -277,6 +283,14 @@ struct MXTensor {
return size;
}
+ /*! \brief helper function to compare two MXTensors */
+ inline bool isSame(const MXTensor &oth) {
Review comment:
I think I agree with @wkcn here, == should compare the values of the tensor not the "state" of the tensor (data_ptr, versionID, etc)
@mseth10 @eric-haibin-lin @haojin2 what do you guys think?
----------------------------------------------------------------
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] samskalicky edited a comment on issue #17204:
Enhancements for MXTensor for custom operators
Posted by GitBox <gi...@apache.org>.
samskalicky edited a comment on issue #17204: Enhancements for MXTensor for custom operators
URL: https://github.com/apache/incubator-mxnet/pull/17204#issuecomment-570619946
@rondogency @mseth10 @wkcn @junrushao1994 for review
----------------------------------------------------------------
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