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

[MXNET-1407] Added test to verify Large Tensor Support for pad operator #15126

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

access2rohit
Copy link
Contributor

@access2rohit access2rohit commented Jun 2, 2019

Description

new operator supported: pad

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-1407], where 1407 refers to the relevant JIRA issue 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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@access2rohit
Copy link
Contributor Author

@mxnet-label-bot add [pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Jun 2, 2019
@access2rohit access2rohit force-pushed the pad_lts branch 2 times, most recently from e035f0c to 00ec663 Compare June 3, 2019 23:29
@vandanavk
Copy link
Contributor

@access2rohit could you check the CI and merge conflict?

@roywei
Copy link
Member

roywei commented Jul 8, 2019

@access2rohit could you rebase this PR? thanks

@karan6181
Copy link
Contributor

@access2rohit Could you please re-trigger the CI and resolve the merge conflicts?

@piyushghai
Copy link
Contributor

@access2rohit Gentle ping...

Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

minor clarification needed

@ChaiBapchya
Copy link
Contributor

@access2rohit gentle ping!

@access2rohit access2rohit changed the title [MXNET-1407] Added test to verify Large Tensor Support for pad operator [WIP][MXNET-1407] Added test to verify Large Tensor Support for pad operator Oct 9, 2019
@access2rohit access2rohit changed the title [WIP][MXNET-1407] Added test to verify Large Tensor Support for pad operator [MXNET-1407] Added test to verify Large Tensor Support for pad operator Oct 15, 2019
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

LGTM

@access2rohit
Copy link
Contributor Author

@sxjscience @apeforest @anirudh2290 @zheng-da @pengzhao-intel This PR is ready for review

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;
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

@anirudh2290
Copy link
Member

Please open an issue for the missing tests and remaining todos in Large Tensor Support.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants