New set default dtype by JiangZhaoh · Pull Request #18251 · apache/mxnet

JiangZhaoh · 2020-05-06T17:12:10Z

Description

#17283 cause an issue #18193. So I depart dtype flag from npx.set_np() in this pull request.
And also try to fix #18060 .

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 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

mxnet-bot · 2020-05-06T17:12:12Z

Hey @JiangZhaoh , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

To trigger all jobs: @mxnet-bot run ci [all]
To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [unix-cpu, website, windows-cpu, windows-gpu, edge, centos-cpu, sanity, unix-gpu, clang, centos-gpu, miscellaneous]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

JiangZhaoh · 2020-05-12T12:11:45Z

@mxnet-bot run ci [windows-cpu]

mxnet-bot · 2020-05-12T12:11:52Z

Jenkins CI successfully triggered : [windows-cpu]

JiangZhaoh · 2020-05-12T12:20:41Z

@mxnet-bot run ci [unix-gpu]

mxnet-bot · 2020-05-12T12:20:46Z

Jenkins CI successfully triggered : [unix-gpu]

JiangZhaoh · 2020-05-12T14:00:49Z

@mxnet-bot run ci [unix-cpu]

mxnet-bot · 2020-05-12T14:00:54Z

Jenkins CI successfully triggered : [unix-cpu]

JiangZhaoh · 2020-05-13T05:01:39Z

@mxnet-bot run ci [unix-gpu]

mxnet-bot · 2020-05-13T05:01:44Z

Jenkins CI successfully triggered : [unix-gpu]

yzhliu

@sxjscience could you also take a look?

sxjscience · 2020-05-14T01:57:42Z

  param.infer_range = false;
  if (args[3].type_code() == kNull) {
-    param.dtype = mshadow::kFloat32;
+    param.dtype = mxnet::common::GetDefaultDtype();


I just realized that in numpy, the default dtype of arange is int64.

import numpy as np print(np.arange(10).dtype) import mxnet as mx mx.npx.set_np() print(mx.np.arange(10).dtype)

Output:

int64 float32

Thus, we should change the dtype to be consistent with the official numpy. What do you think @yzhliu @leezu

I just realized that in numpy, the default dtype of arange is int64.

import numpy as np print(np.arange(10).dtype) import mxnet as mx mx.npx.set_np() print(mx.np.arange(10).dtype)

Output:

int64 float32

Thus, we should change the dtype to be consistent with the official numpy. What do you think @yzhliu @leezu

I think maybe we could consider the most common use cases to decide which return result to choose.

In fact, arange is usually used for constructing the index. Sometimes, the user may use np.arange(0, 1000*1000*1000, 10000) and we will lose precision if we use float32. In addition, pytorch uses int64.

import torch as th print(th.arange(10).dtype) # torch.int64

The example @sxjscience listed looks reasonable to me. I guess we should do int64 then.

yzhliu · 2020-05-18T08:24:48Z

otherwise good to me.

JiangZhaoh · 2020-05-18T23:06:17Z

@mxnet-bot run ci [centos-cpu, miscellaneous, unix-cpu]

mxnet-bot · 2020-05-18T23:06:26Z

Jenkins CI successfully triggered : [unix-cpu, centos-cpu, miscellaneous]

JiangZhaoh · 2020-05-19T00:09:29Z

@mxnet-bot run ci [centos-cpu]

mxnet-bot · 2020-05-19T00:09:34Z

Jenkins CI successfully triggered : [centos-cpu]

yzhliu · 2020-05-19T23:39:01Z

Thanks @JiangZhaoh @sxjscience

* apply apache#17283 * fix issue apache#18060 * fix error * remove redundant code * fix CI error * replace Flase to False * add 'dtype=False' to set_np() * fix doc * default 'arange' default np dtype as int64

JiangZhaoh requested review from eric-haibin-lin and szha as code owners May 6, 2020 17:12

JiangZhaoh added 5 commits May 12, 2020 08:17

apply apache#17283

2bdc27f

fix issue apache#18060

846ede7

fix error

031dc5d

remove redundant code

357be34

fix CI error

e7f0f36

JiangZhaoh force-pushed the new_set_default_dtype branch from 9faab79 to e7f0f36 Compare May 12, 2020 10:18

sxjscience mentioned this pull request May 13, 2020

different dtype when calculating division compared with numpy #18297

Closed

yzhliu reviewed May 13, 2020

View reviewed changes

Comment thread python/mxnet/ndarray/numpy/random.py Outdated

Comment thread python/mxnet/util.py Outdated

JiangZhaoh added 2 commits May 14, 2020 00:00

replace Flase to False

1584744

add 'dtype=False' to set_np()

156e88b

sxjscience reviewed May 14, 2020

View reviewed changes

Comment thread python/mxnet/util.py Outdated

sxjscience reviewed May 14, 2020

View reviewed changes

Comment thread python/mxnet/util.py Outdated

fix doc

692f89b

sxjscience reviewed May 14, 2020

View reviewed changes

default 'arange' default np dtype as int64

a5ce8dc

yzhliu approved these changes May 19, 2020

View reviewed changes

yzhliu merged commit b904d48 into apache:master May 19, 2020

yzhliu mentioned this pull request Jun 3, 2020

Array initialization and indexing is inconsistent with official numpy #16991

Closed

sxjscience mentioned this pull request Jul 1, 2020

[v1.7.x] Backport some numpy features + fixes #18648

Closed

7 tasks

Conversation

JiangZhaoh commented May 6, 2020

Description

Checklist

Essentials

Changes

Comments

Uh oh!

mxnet-bot commented May 6, 2020

Uh oh!

JiangZhaoh commented May 12, 2020

Uh oh!

mxnet-bot commented May 12, 2020

Uh oh!

JiangZhaoh commented May 12, 2020

Uh oh!

mxnet-bot commented May 12, 2020

Uh oh!

JiangZhaoh commented May 12, 2020

Uh oh!

mxnet-bot commented May 12, 2020

Uh oh!

JiangZhaoh commented May 13, 2020

Uh oh!

mxnet-bot commented May 13, 2020

Uh oh!

yzhliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sxjscience May 14, 2020

Choose a reason for hiding this comment

Uh oh!

JiangZhaoh May 17, 2020

Choose a reason for hiding this comment

Uh oh!

sxjscience May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yzhliu May 18, 2020

Choose a reason for hiding this comment

Uh oh!

yzhliu commented May 18, 2020

Uh oh!

JiangZhaoh commented May 18, 2020

Uh oh!

mxnet-bot commented May 18, 2020

Uh oh!

JiangZhaoh commented May 19, 2020

Uh oh!

mxnet-bot commented May 19, 2020

Uh oh!

yzhliu commented May 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sxjscience May 17, 2020 •

edited

Loading