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

adding docs for 64bit C APIs of large tensor and removing not required int64 C APIs #17309

Merged
merged 1 commit into from
Jan 18, 2020
Merged

adding docs for 64bit C APIs of large tensor and removing not required int64 C APIs #17309

merged 1 commit into from
Jan 18, 2020

Conversation

access2rohit
Copy link
Contributor

Description

adding docs for 64bit C APIs of large tensor and removing not required int64 C APIs:
MXNDArrayGetShape64
MXSymbolInferShape64
MXSymbolInferShapePartial64

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • 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
  • To the best of my 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]

@lanking520 lanking520 added the pr-awaiting-review PR is waiting for code review label Jan 14, 2020
@access2rohit
Copy link
Contributor Author

@apeforest @ChaiBapchya PR ready for review

apeforest
apeforest reviewed Jan 14, 2020
View reviewed changes
include/mxnet/c_api.h
@@ -585,6 +585,8 @@ MXNET_DLL int MXNDArrayCreate(const uint32_t *shape,

/*!
* \brief create a NDArray with specified shape and data type
* This api is available when MXNet is built with flag
* USE_INT64_TENSOR_SIZE=0 (by default)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of adding the comment, we should just error out from the API when the incorrect flags are used with API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There already is a functionality to throw error messages when usage is of large tensor and mxnet is not built with USE_INT64_TENSOR_SIZE=1. This is a documentation for devs to help them decide which one to choose.

ChaiBapchya
ChaiBapchya approved these changes Jan 14, 2020
View reviewed changes
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.

nitpicks. Rest looks good!

include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h
* \param out_shape_size sizeof the returning array of out_shapes
* \param out_shape_ndim returning array of shape dimensions of eachs input shape.
* \param out_shape_data returning array of pointers to head of the input shape.
* \param aux_shape_size sizeof the returning array of aux_shapes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* \param aux_shape_size sizeof the returning array of aux_shapes
* \param aux_shape_size size of the returning array of aux_shapes

Copy link
Contributor Author

@access2rohit access2rohit Jan 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

addressed all comments. sizeof can also be also thought of as a function, which I think is fine. So, decided to keep it that way. Let me know if its important to make it "size of". I don't have any strong opinion on keeping it "sizeof"

include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h Outdated Show resolved Hide resolved
ChaiBapchya
ChaiBapchya reviewed Jan 14, 2020
View reviewed changes
include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h Outdated Show resolved Hide resolved
include/mxnet/c_api.h Outdated Show resolved Hide resolved
@apeforest apeforest merged commit 5e64e96 into apache:master Jan 18, 2020
@szha
Copy link
Member

szha commented Jan 19, 2020

@access2rohit @apeforest could you help clarify which version the *64 APIs were first introduced? If they appeared in the past versions before, then this change cannot be merged into 1.7.x (#16864)

@apeforest
Copy link
Contributor

I think the *64APIs were introduce in 1.6. @access2rohit Please confirm. Thanks.

szhengac pushed a commit to szhengac/mxnet that referenced this pull request Jan 21, 2020
@access2rohit
adding docs for 64bit C APIs of large tensor (apache#17309)
5d93c00
szhengac pushed a commit to szhengac/mxnet that referenced this pull request Jan 21, 2020
@access2rohit
adding docs for 64bit C APIs of large tensor (apache#17309)
f1a2f8d
szhengac pushed a commit to szhengac/mxnet that referenced this pull request Jan 21, 2020
@access2rohit
adding docs for 64bit C APIs of large tensor (apache#17309)
8f14ec1
@access2rohit
Copy link
Contributor Author

I think the *64APIs were introduce in 1.6. @access2rohit Please confirm. Thanks.

Correct !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ChaiBapchya ChaiBapchya ChaiBapchya approved these changes

@apeforest apeforest apeforest approved these changes

@anirudh2290 anirudh2290 Awaiting requested review from anirudh2290

@eric-haibin-lin eric-haibin-lin Awaiting requested review from eric-haibin-lin eric-haibin-lin is a code owner

Assignees
No one assigned
Labels
API change pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@access2rohit @szha @apeforest @ChaiBapchya @lanking520