-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-1407] Added test to verify Large Tensor Support for pad operator #15126
Conversation
@mxnet-label-bot add [pr-awaiting-review] |
e035f0c
to
00ec663
Compare
@access2rohit could you check the CI and merge conflict? |
@access2rohit could you rebase this PR? thanks |
@access2rohit Could you please re-trigger the CI and resolve the merge conflicts? |
@access2rohit Gentle ping... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor clarification needed
@access2rohit gentle ping! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@sxjscience @apeforest @anirudh2290 @zheng-da @pengzhao-intel This PR is ready for review |
f89f680
to
7c6b2ee
Compare
src/operator/pad.cc
Outdated
index_t iStartZ = std::max(index_t{0}, -pad_f); | ||
index_t oStartX = std::max(index_t{0}, pad_l); | ||
index_t oStartY = std::max(index_t{0}, pad_t); | ||
index_t oStartZ = std::max(index_t{0}, pad_f); | ||
|
||
int l, ip_x, ip_y, ip_z; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these not changed to index_t. also why is i, j, k inside pragma omp parallel not changed to index_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch ... missed it. Will add it now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a test, if the current test missed catching this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added tests for backward pass since initial large tensor support only works for forward passes.
Please open an issue for the missing tests and remaining todos in Large Tensor Support. |
Description
new operator supported: pad
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.