-
Notifications
You must be signed in to change notification settings - Fork 432
Conversation
@eric-haibin-lin @tqchen @piiswrong Please review this PR for large array support in MXNet. Thx. |
If we are going to change this we should change it to signed int64_t instead of unsigned size_t. Although there could be pitfalls. @tqchen any thoughts? |
int64_t is a better choice than size_t |
@tqchen Could you elaborate a little more on the advantage of using int64_t over size_t here? Curious to learn. Thanks! |
The main reason is that int64_t is used by default by DLPack and a few other frameworks. It also allows introducing -1 as some special placeholder when necessary. size_t is usually not preferred in data-structure members, because it has a cross-platform issue(size_t can be 32 bit in 32bit platforms). I have explained the reason to favor int64_t over uint64_t |
@tqchen Thanks for your explanation. I have change to use int64_t as type for the tensor dimension. For the completeness of this change, instead of only changing return type of the I have written simple example to check runtime impact to softmax and relu operators in MXNet and did not see noticeable difference. Any other suggestion on performance test is appreciated.
|
@tqchen I will appreciate if you could help to review this at your convenience or relay to other reviewers for me. We have another PR (apache/mxnet#11742) dependent on this. Thanks! |
@piiswrong @tqchen Would appreciate if you could help review this at your earliest convenience. There is a MXNet issue depending on this. Thanks! |
This reverts commit d68d369.
Revert "Allow large array operation in MXNet (#348)"
Re-Revert PR #348 to support large tensor size in MXNet
This change is a dependency for the integer overflow issue in MXNet where a large ND array is created and converted to numpy.
An earlier PR was reviewed but it makes too significant change to mshadow module and is not necessary. This PR is a new one and it has been verified in the MXNet issue filed above.