Typedef cleanup in C API #15899

access2rohit · 2019-08-15T02:39:28Z

Description

changes

mx_uint -> uint32_t
mx_float -> float

removes

mx_int64 (Since introduced only by large tensor support so not present in other language bindings)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID 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)
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
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

access2rohit · 2019-08-15T02:39:50Z

@mxnet-label-bot add [pr-work-in-progress]

access2rohit · 2019-08-19T07:43:14Z

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

access2rohit · 2019-08-19T07:43:31Z

@apeforest @ChaiBapchya pr is ready to review

ChaiBapchya

LGTM!

apeforest · 2019-08-20T05:20:43Z

include/mxnet/c_predict_api.h

@@ -43,8 +43,6 @@ extern "C" {

 /*! \brief manually define unsigned int */
 typedef uint32_t mx_uint;


Why do we still need this?

yes we do other language bindings use this

Which language bindings use this?

java, C++, R, Scala

Can you point me to the line in this bindings? It looks weird if this typedef is not used in this file while still keeping it.

Few examples in code:

https://github.com/apache/incubator-mxnet/blob/3f3ba92ae1468d08de088d2291ca14e2d5dc5515/perl-package/AI-MXNetCAPI/mxnet_typemaps.i#L171

https://github.com/apache/incubator-mxnet/blob/3f3ba92ae1468d08de088d2291ca14e2d5dc5515/R-package/src/ndarray.cc#L34

https://github.com/apache/incubator-mxnet/blob/b88705e7744be1cc22122eb2c5b492e495ab45bb/scala-package/native/src/main/native/org_apache_mxnet_native_c_api.cc#L89

apeforest · 2019-08-20T05:20:53Z

include/mxnet/c_predict_api.h

@@ -43,8 +43,6 @@ extern "C" {

 /*! \brief manually define unsigned int */
 typedef uint32_t mx_uint;
-/*! \brief manually define 64-bit int */
-typedef int64_t mx_int64;
 /*! \brief manually define float */
 typedef float mx_float;


Why do we still need this?

yes we do other language bindings use this

Which language bindings use this?

java, C++, R, Scala

I did a search in MXNet codebase and could not find any. Could you point me to the line?

apeforest

Thanks for the refactoring. This will increase readability and make C API users less dependency as well. I guess you also need to update the python interface that invokes the C API?

access2rohit · 2019-08-20T05:44:04Z

Thanks for the refactoring. This will increase readability and make C API users less dependency as well. I guess you also need to update the python interface that invokes the C API?

@apeforest This PR only contains code cleanup from C API end. Python cleanup needs to be done in a separate PR. typedefs should remain as is because other language bindings use them.

apeforest · 2019-08-20T05:50:46Z

Thanks for the refactoring. This will increase readability and make C API users less dependency as well. I guess you also need to update the python interface that invokes the C API?

@apeforest This PR only contains code cleanup from C API end. Python cleanup needs to be done in a separate PR. typedefs should remain as is because other language bindings use them.

Why can't it be done in one PR? Isn't that what the purpose of this PR is?

access2rohit · 2019-08-20T05:59:57Z

Thanks for the refactoring. This will increase readability and make C API users less dependency as well. I guess you also need to update the python interface that invokes the C API?

@apeforest This PR only contains code cleanup from C API end. Python cleanup needs to be done in a separate PR. typedefs should remain as is because other language bindings use them.

Why can't it be done in one PR? Isn't that what the purpose of this PR is?

These are independent changes. I think these can be done in separate PRs. This is also keep C changes and individual language binding related changes separate, keeping each PR small

* change mx_uint->int32_t * changing mx_int64 -> int64_t * change mx_float -> float * maintining typedefs for other language bindings * Re-Trigger build

access2rohit requested review from anirudh2290 and eric-haibin-lin as code owners August 15, 2019 02:39

marcoabreu added the pr-work-in-progress PR is still work in progress label Aug 15, 2019

access2rohit force-pushed the typedef_cleanup branch 2 times, most recently from 99cb9d8 to 957cda4 Compare August 19, 2019 07:42

access2rohit changed the title ~~[WIP]Typedef cleanup~~ Typedef cleanup Aug 19, 2019

marcoabreu added the pr-awaiting-review PR is waiting for code review label Aug 19, 2019

ChaiBapchya approved these changes Aug 19, 2019

View reviewed changes

access2rohit force-pushed the typedef_cleanup branch from 957cda4 to d38aeb7 Compare August 19, 2019 22:37

apeforest reviewed Aug 20, 2019

View reviewed changes

access2rohit force-pushed the typedef_cleanup branch from d38aeb7 to 9bfb654 Compare August 20, 2019 05:57

access2rohit force-pushed the typedef_cleanup branch from 9bfb654 to 6763a08 Compare August 21, 2019 08:32

Rohit Kumar Srivastava added 4 commits August 31, 2019 20:44

change mx_uint->int32_t

3f1415c

changing mx_int64 -> int64_t

b708354

change mx_float -> float

3693100

maintining typedefs for other language bindings

2ce1c21

access2rohit force-pushed the typedef_cleanup branch from b0f7545 to 2ce1c21 Compare August 31, 2019 20:45

Re-Trigger build

1960ec0

apeforest approved these changes Sep 3, 2019

View reviewed changes

apeforest merged commit 5b301c6 into apache:master Sep 3, 2019

access2rohit changed the title ~~Typedef cleanup~~ Typedef cleanup in C API Sep 3, 2019

gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019

Typedef cleanup (apache#15899)

988ffed

* change mx_uint->int32_t * changing mx_int64 -> int64_t * change mx_float -> float * maintining typedefs for other language bindings * Re-Trigger build

gyshi pushed a commit to gyshi/incubator-mxnet that referenced this pull request Sep 7, 2019

Typedef cleanup (apache#15899)

35a767f

* change mx_uint->int32_t * changing mx_int64 -> int64_t * change mx_float -> float * maintining typedefs for other language bindings * Re-Trigger build

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typedef cleanup in C API #15899

Typedef cleanup in C API #15899

access2rohit commented Aug 15, 2019

access2rohit commented Aug 15, 2019

access2rohit commented Aug 19, 2019

access2rohit commented Aug 19, 2019

ChaiBapchya left a comment

apeforest Aug 20, 2019

access2rohit Aug 20, 2019

apeforest Aug 20, 2019

access2rohit Aug 20, 2019

apeforest Aug 26, 2019

access2rohit Aug 31, 2019 •

edited

Loading

apeforest Aug 20, 2019

access2rohit Aug 20, 2019

apeforest Aug 20, 2019

access2rohit Aug 20, 2019

apeforest Aug 26, 2019

apeforest left a comment •

edited

Loading

access2rohit commented Aug 20, 2019

apeforest commented Aug 20, 2019

access2rohit commented Aug 20, 2019

		@@ -43,8 +43,6 @@ extern "C" {

		/! \brief manually define unsigned int /
		typedef uint32_t mx_uint;

Typedef cleanup in C API #15899

Typedef cleanup in C API #15899

Conversation

access2rohit commented Aug 15, 2019

Description

Checklist

Essentials

access2rohit commented Aug 15, 2019

access2rohit commented Aug 19, 2019

access2rohit commented Aug 19, 2019

ChaiBapchya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

access2rohit Aug 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apeforest left a comment • edited Loading

Choose a reason for hiding this comment

access2rohit commented Aug 20, 2019

apeforest commented Aug 20, 2019

access2rohit commented Aug 20, 2019

access2rohit Aug 31, 2019 •

edited

Loading

apeforest left a comment •

edited

Loading