You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mxnet.apache.org by GitBox <gi...@apache.org> on 2020/01/09 08:01:03 UTC

[GitHub] [incubator-mxnet] JiangZhaoh opened a new pull request #17255: Set np default dtype( float32 <-> float64)

JiangZhaoh opened a new pull request #17255: Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255
 
 
   ## Description ##
   (Brief description on what this PR is about)
   
   ## 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](https://issues.apache.org/jira/projects/MXNET/issues) 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 https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
   - [ ] To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
   
   ### Changes ###
   - [ ] Feature1, tests, (and when applicable, API doc)
   - [ ] Feature2, tests, (and when applicable, API doc)
   
   ## Comments ##
   - If this change is a backward incompatible change, why must this change be made.
   - Interesting edge cases to note here
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572464020
 
 
   My question is rather towards whether the functionality itself will provide a good user experience - the implementation is of secondary nature at this point. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572474784
 
 
   Thanks for elaborating, makes sense.
   
   Merging it into set_np sounds like a good addition, indeed.
   
   I know that the code is not ready for review yet, I still have a gut feeling that we are approaching this from a strange angle. Sometimes, the dtype parameter is removed, sometimes it's set to float, sometimes to none and there are probably more cases. 
   
   Do we have a concept how exactly we'd like to have this introduced? Right now it feels a bit cluttered and too many things are changed at the same time in different ways.
   
   Not trying to criticize here, just trying to give a bit of early input from an external view.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#discussion_r364606241
 
 

 ##########
 File path: include/mxnet/imperative.h
 ##########
 @@ -136,6 +137,34 @@ class Imperative {
     }
     return old;
   }
+  /*! \brief return current ndarray default dtype,
+   *  float32(false), float64(true).
+   * */
+  int is_np_default_dtype() const {
+    if (is_np_default_dtype_global_) {
+      return 2;
 
 Review comment:
   Could you use an enum instead?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] JiangZhaoh commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
JiangZhaoh commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572460667
 
 
   > I don't know... Somehow this change feels wrong to me - a default is a default and people can always specify the dtype if they would like to have a particular one. This PR changes so many things and it feels odd. I'm afraid it will cause quite some confusion when people explicitly set the type, change the default and then wonder when it's not being used - the UX wouldn't be great
   
   Could you please tell me how can I finish this function if I don't change these files? I know some files maybe don't need to change, but I think most of them definitely need to be modified to assure consistency. If you have better idea, please explain it and I'm willing to change my method.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572472159
 
 
   @marcoabreu
   Let's probably take a look at this example to help you understand:
   ```python
   import numpy as np
   a = np.ones((4096, 1024)) / 1000000
   # original code, a is of type float64
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   a = np.ones((4096, 1024)) / 1000000
   # a is of type float32, which might not be able to accurately represent very small numbers,
   # could possibly cause some precision errors in later computations
   a = np.ones((4096, 1024), dtype=np.float64) / 1000000
   # to recover the same precision, need to touch the original code to explicitly specify the dtype,
   # number of lines changed is the same as number of array creation functions used with in the whole file.
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   npx.set_np_default_type()
   # switch the default type, this function call could be further merged to be part of set_np function
   a = np.ones((4096, 1024)) / 1000000
   # now a is of type float64, not interfering with the original assumption of precisions without any changes to the original code.
   # only one more line is added, not the same as the total # of array creations in the script.
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572472159
 
 
   @marcoabreu
   Let's probably take a look at this example to help you understand:
   ```python
   import numpy as np
   a = np.ones((4096, 1024)) / 1000000
   # original code, a is of type float64
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   a = np.ones((4096, 1024)) / 1000000
   # a is of type float32, which might not be able to accurately represent very small numbers, could possibly cause some precision errors in later computations
   a = np.ones((4096, 1024), dtype=np.float64) / 1000000
   # to recover the same precision, need to touch the original code to explicitly specify the dtype,
   # number of lines changed is the same as number of array creation functions used with in the whole file.
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   npx.set_np_default_type()
   # switch the default type, this function call could be further merged to be part of set_np function
   a = np.ones((4096, 1024)) / 1000000
   # now a is of type float64, not interfering with the original assumption of precisions without any changes to the original code.
   # only one more line is added, not the same as the total # of array creations in the script.
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] JiangZhaoh commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
JiangZhaoh commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572460479
 
 
   > This PR changes so many things and it feels odd. I'm afraid it will cause quite some confusion when people explicitly set the type, change the default and then wonder when it's not being used - the UX wouldn't be great
   Could you please tell me how can I finish this function if I don't change these files? I know some files maybe don't need to change, but I think most of them definitely need to be modified to assure consistency. If you have better idea, please explain it and I'm willing to change my method.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#discussion_r364606558
 
 

 ##########
 File path: include/mxnet/imperative.h
 ##########
 @@ -136,6 +137,34 @@ class Imperative {
     }
     return old;
   }
+  /*! \brief return current ndarray default dtype,
+   *  float32(false), float64(true).
+   * */
+  int is_np_default_dtype() const {
+    if (is_np_default_dtype_global_) {
+      return 2;
+    }
+    return is_np_default_dtype_thread_local_ ? 1 : 0;
+  }
+  /*! \brief specify ndarray default dtype off, thread local on or global on. */
+  bool set_is_np_default_dtype(int is_np_default_dtype) {
+    NumpyDefaultDtype flag = static_cast<NumpyDefaultDtype>(is_np_default_dtype);
+    bool old = this->is_np_default_dtype();
 
 Review comment:
   Type mismatch

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572472159
 
 
   @marcoabreu
   Let's probably take a look at this example to help you understand:
   ```python
   import numpy as np
   a = np.ones((4096, 1024)) / 1000000
   # original code, a is of type float64
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   a = np.ones((4096, 1024)) / 1000000
   # a is of type float32, which might not be able to accurately represent very small numbers,
   # could possibly cause some precision errors in later computations
   a = np.ones((4096, 1024), dtype=np.float64) / 1000000
   # to recover the same precision, need to touch the original code to explicitly specify the dtype,
   # number of lines changed is the same as number of array creation functions used with in the whole file.
   ```
   ```python
   from mxnet import np, npx
   npx.set_np()
   # transitioning to NP on MXNet, swapping the library
   npx.set_np_default_type()
   # switch the default type, this function call could be further merged to be part of set_np function
   # to even avoid this extra line
   a = np.ones((4096, 1024)) / 1000000
   # now a is of type float64, not interfering with the original assumption of precisions without any changes to the original code.
   # only one more line is added, not the same as the total # of array creations in the script.
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572477944
 
 
   @marcoabreu It's really case by case for each op, it should only affect array creation ops, and this is not the final version yet. I think we need to clean up the code that assign the default types in the python frontend to go with this newly-defined behavior. Thanks for you input

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] JiangZhaoh closed pull request #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
JiangZhaoh closed pull request #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] JiangZhaoh removed a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
JiangZhaoh removed a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572460479
 
 
   > This PR changes so many things and it feels odd. I'm afraid it will cause quite some confusion when people explicitly set the type, change the default and then wonder when it's not being used - the UX wouldn't be great
   Could you please tell me how can I finish this function if I don't change these files? I know some files maybe don't need to change, but I think most of them definitely need to be modified to assure consistency. If you have better idea, please explain it and I'm willing to change my method.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572466288
 
 
   Okay, so there's a functionality in numpy to set the default dtype or is this approach chosen as a compromise to not alter the array creation functions while still offering the possibility to have  parity with numpy?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 commented on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572465096
 
 
   @marcoabreu 
   If you explicitly specified some dtype in an op with a `dtype` option, it means that you're not expecting to see whatever the default dtype is, isn't that a very natural thing? I'm not seeing any UX problem following this kind of logic.
   This new default type option is part of the new NumPy-compatible user experience for a closer compatibility with existing code written in NumPy, a lot of which pose the assumptions that array creation functions return arrays with float64 data type by default, and failure to meet such assumption could lead to crashes and unexpected numerical bugs. For a more smooth transition for those code to NP on MXNet, providing such an option instead of having to change all array creation functions in those code to explicitly say `dtype=np.float64` would, I assume, be providing a better UX.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
haojin2 edited a comment on issue #17255: [DO NOT REVIEW]Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#issuecomment-572477944
 
 
   @marcoabreu It's really case by case for each op, it should only affect array creation ops, and this is not the final version yet. I think we need to clean up the code that assign the default types in the python frontend to go with this newly-defined behavior. Thanks for your input

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
marcoabreu commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#discussion_r364606683
 
 

 ##########
 File path: include/mxnet/imperative.h
 ##########
 @@ -136,6 +137,34 @@ class Imperative {
     }
     return old;
   }
+  /*! \brief return current ndarray default dtype,
+   *  float32(false), float64(true).
+   * */
+  int is_np_default_dtype() const {
+    if (is_np_default_dtype_global_) {
+      return 2;
+    }
+    return is_np_default_dtype_thread_local_ ? 1 : 0;
+  }
+  /*! \brief specify ndarray default dtype off, thread local on or global on. */
+  bool set_is_np_default_dtype(int is_np_default_dtype) {
+    NumpyDefaultDtype flag = static_cast<NumpyDefaultDtype>(is_np_default_dtype);
+    bool old = this->is_np_default_dtype();
+    switch (flag) {
+      case GlobalOn:
+        is_np_default_dtype_global_ = true;
+        is_np_default_dtype_thread_local_ = true;
+        break;
+      case ThreadLocalOn:
+        is_np_default_dtype_thread_local_ = true;
+        break;
+      case Off:
+        is_np_default_dtype_global_ = false;
+        is_np_default_dtype_thread_local_ = false;
+        break;
+    }
+    return old;
 
 Review comment:
   ?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-mxnet] JiangZhaoh commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)

Posted by GitBox <gi...@apache.org>.
JiangZhaoh commented on a change in pull request #17255: Set np default dtype( float32 <-> float64)
URL: https://github.com/apache/incubator-mxnet/pull/17255#discussion_r364612206
 
 

 ##########
 File path: include/mxnet/imperative.h
 ##########
 @@ -136,6 +137,34 @@ class Imperative {
     }
     return old;
   }
+  /*! \brief return current ndarray default dtype,
+   *  float32(false), float64(true).
+   * */
+  int is_np_default_dtype() const {
+    if (is_np_default_dtype_global_) {
+      return 2;
 
 Review comment:
   Sure.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services