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

added large tensor support for add_n and tests for more ops #16476

Merged
merged 1 commit into from
Oct 18, 2019

Conversation

access2rohit
Copy link
Contributor

Description

Added large tensor support for add_n and tests for:
load()
save()
add_n()
modulo()
maximum()
minimum()

Checklist

Essentials

  • 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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Testing

Will run full suite of tests and paste here.

@access2rohit access2rohit changed the title adding large tensor support for add_n and tests for more ops added large tensor support for add_n and tests for more ops Oct 16, 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 with minor nitpicks.

@access2rohit
Copy link
Contributor Author

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

src/operator/tensor/elemwise_sum.h Show resolved Hide resolved
assert z[-1] == 2
z = nd.minimum(x, 5)
assert z[0] == 3
assert z[-1] == 3
Copy link
Member

Choose a reason for hiding this comment

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

are the backward pass of any of these functions tested.

Copy link
Contributor Author

@access2rohit access2rohit Oct 17, 2019

Choose a reason for hiding this comment

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

Only forward passes are required by DGL. So only forward passes are tested.

Copy link
Member

Choose a reason for hiding this comment

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

is there some large tensor docs in mxnet website. That should explicitly call out that the backward pass has not been verified with large tensor.

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 have to write that. Currently we don't have any docs explaining how to use large tensor support and caveats along with limitations and perf regressions.

@ChaiBapchya
Copy link
Contributor

@anirudh2290 good to merge?

@access2rohit
Copy link
Contributor Author

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

@lanking520 lanking520 added the pr-awaiting-merge Review and CI is complete. Ready to Merge label Oct 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants