[WIP] fix different round behavior on CPU and GPU #16498 #17443

musikisomorphie · 2019-02-24T07:56:11Z

@xxtemp, @colesbury, @bhushan23, @zou3519, convert gpu round behavior to half-to-even, consistent with torch cpu version and numpy. You feedback are welcomed.
See #16498

bhushan23 · 2019-02-24T23:31:45Z

can you also add test cases?

soumith · 2019-02-25T04:23:30Z

yes, add a test case to test/test_cuda.py and this should be good to go

vishwakftw · 2019-02-25T05:30:28Z

I think this doesn't address the issue with respect to CPU rounding, which is still different based on the instruction set. See #16498 (comment)

bhushan23

I thought only one blank line is allowed across functions

musikisomorphie · 2019-02-26T05:33:44Z

I thought only one blank line is allowed across functions

I saw the error mentioning that there is one blank line missing, that is why I added it.
I guess since the indentation is different between test_cuda_round() and load_ignore_file(), two blank lines are required.

musikisomorphie · 2019-02-26T05:53:09Z

@soumith, a unit test is added.

@vishwakftw What do you mean "instruction set"? I didn't see the case that the cpu implementation of torch.round() wraps std::round. Besides, based on my test, the cpu version of torch.round() follows half-to-even mode. If you have counterexamples, please let me know.

vishwakftw · 2019-02-26T07:36:10Z

@musikisomorphie the half-to-even is because of the vectorized implementation, which is what you see in your CPU tests.

std::round(2.5) rounds to 3 (away from zero), whereas the vectorized implementation rounds 2.5 to 2 (nearest even). The patch for this issue would be to wrap the CPU implementation with std::rint instead of std::round.

vishwakftw · 2019-02-26T07:36:32Z

I think it'd be better to label this PR as bc-breaking.

musikisomorphie · 2019-02-26T07:58:35Z

@musikisomorphie the half-to-even is because of the vectorized implementation, which is what you see in your CPU tests.

std::round(2.5) rounds to 3 (away from zero), whereas the vectorized implementation rounds 2.5 to 2 (nearest even). The patch for this issue would be to wrap the CPU implementation with std::rint instead of std::round.

@vishwakftw thank you for your feedback.
What I did here is making the torch.round(), torch.cuda().round() consistent with np.round(), i.e., half-to-even.
I suppose what you expect is replacing all std::round in the pytorch's source code with std:rint, so that the whole framework strictly sticks to half-to-even mode.
There exists mismatch between my revision and your expectation. Unfortunately I may not have enough time to work on the latter.

aa

musikisomorphie · 2019-02-28T10:26:33Z

@bhushan23, @soumith, should we merge this PR or?

facebook-github-bot

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

VitalyFedyunin · 2019-02-28T19:41:59Z

Actually replacing round to nearbyint in:

pytorch/aten/src/ATen/cpu/vec256/vec256_base.h

Line 224 in 1ff8647

return map(std::round);

and TH_MATH_NAME(round) to TH_MATH_NAME(nearbyint) everywhere (just two files)

should suffice.

musikisomorphie · 2019-02-28T20:13:56Z

TH_MATH_NAME(round)

Ok, I will fix them. Thanks for the tip.

facebook-github-bot

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

VitalyFedyunin · 2019-03-01T15:35:51Z

Please merge master into this branch and avoid force-push.

vishwakftw · 2019-03-01T15:38:12Z

Is there any way to test CPU behavior for the non-vectorized version which has been modified?

VitalyFedyunin · 2019-03-01T15:50:47Z

Pretty much no, as all calls now dispatch to the vectorized version. Asking
@musikisomorphie to remove old TH code would be too much for this PR.

The only round() which is not covered by vectorized implementation is HalfTensor(a).cpu().round(), but right now we throw "round_out is not implemented for type torch.HalfTensor" anyway.

musikisomorphie · 2019-03-01T17:22:08Z

Please merge master into this branch and avoid force-push.

Do you mean I should rebase my commit and without using force-push?

VitalyFedyunin · 2019-03-01T19:28:46Z

Just pull most recent master into your repo (do not rebase).

musikisomorphie · 2019-03-01T22:40:47Z

Just pull most recent master into your repo (do not rebase).

I updated my branch, then git push origin my-branch. Is it correct?

bhushan23 · 2019-03-02T00:37:36Z

Yes.
I prefer to keep everything clean and no merge commits ( which will happen here), but that's fine.

musikisomorphie · 2019-03-04T18:55:01Z

Hi guys @VitalyFedyunin, @bhushan23, how is this progress of this PR going?

facebook-github-bot

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

VitalyFedyunin · 2019-03-04T20:14:59Z

@pytorchbot retest this please

VitalyFedyunin · 2019-03-04T22:03:32Z

@pytorchbot retest this please

VitalyFedyunin · 2019-03-05T04:01:29Z

@pytorchbot retest this please

VitalyFedyunin · 2019-03-06T04:24:34Z

@pytorchbot retest this please

VitalyFedyunin · 2019-03-06T15:25:12Z

@pytorchbot rebase this please

pytorchbot · 2019-03-06T15:25:13Z

Sorry, only maintainers are authorized to rebase other people's PRs. Feel free to try again on one of your PRs!

(To learn more about this bot, see Bot commands.)

musikisomorphie · 2019-03-06T20:09:37Z

@pytorchbot rebase this please

Hi @VitalyFedyunin, should I rebase this PR myself?

VitalyFedyunin · 2019-03-06T21:36:30Z

Yes please, there are internal tests failing (where they not supposed to), so I was trying to check if rebase would help.

add cuda round test add a new blank line after test_cuda_round convert all cpu round to nearbyint

musikisomorphie · 2019-03-06T22:15:32Z

Yes please, there are internal tests failing (where they not supposed to), so I was trying to check if rebase would help.

I rebased and force pushed my branch, without force the terminal shows:

To https://github.com/musikisomorphie/pytorch.git
! [rejected] round -> round (non-fast-forward)
error: failed to push some refs to 'https://github.com/musikisomorphie/pytorch.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

facebook-github-bot

@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: xxtemp, colesbury, bhushan23, zou3519, convert gpu round behavior to half-to-even, consistent with torch cpu version and numpy. You feedback are welcomed. See #16498 Pull Request resolved: pytorch/pytorch#17443 Differential Revision: D14261786 Pulled By: VitalyFedyunin fbshipit-source-id: 98156436b545d72769831a89e2775d43ad913ebc

* upstream/master: (24 commits) Automatic update of fbcode/onnx to 96c58ceeacf0f2b73d752e413e4fd78787a12da3 (pytorch#17676) Set the default ONNX opset to the latest stable opset (i.e., 9) (pytorch#17736) Add module attributes (pytorch#17309) - refactoring serialization of ONNX initializers to be name-based (pytorch#17420) ONNX Export for Max and Average Pooling in CEIL_MODE use flake8-mypy (pytorch#17721) use fp16<->fp32 intrinsic (pytorch#17496) Implement a Caffe2 standalone LSTM operator (pytorch#17726) caffe2:libtorch_cuda depends on caffe2:caffe2_gpu (pytorch#17729) add tensor and cost inference functions (pytorch#17684) ONNX Export Narrow op Keep the dim_type of hinted shape as BATCH if possible (pytorch#17734) fix different round behavior on CPU and GPU pytorch#16498 (pytorch#17443) Warn about memory overlaps on expanded tensors (pytorch#17576) fix exp fam. formula refactor caffe2 operator constructors - 10/9 (pytorch#17659) Improve ONNX symbolic for logsoftmax and softmax (pytorch#17672) Enable using CMD when building cpp extensions on Windows Do not rename net boundary inputs/outputs during ssaRewrite. (pytorch#17545) Reapply D14078519 (pytorch#17596) ...

musikisomorphie changed the title ~~convert cuda round to half to even~~ fix different round behavior on CPU and GPU #16498 Feb 24, 2019

ezyang changed the title ~~fix different round behavior on CPU and GPU #16498~~ [WIP] fix different round behavior on CPU and GPU #16498 Feb 25, 2019

bhushan23 reviewed Feb 26, 2019

View reviewed changes

musikisomorphie closed this Feb 26, 2019

musikisomorphie reopened this Feb 26, 2019

Merge pull request #1 from pytorch/master

901dc78

aa

facebook-github-bot reviewed Feb 28, 2019

View reviewed changes

VitalyFedyunin self-requested a review February 28, 2019 17:11

facebook-github-bot reviewed Feb 28, 2019

View reviewed changes

VitalyFedyunin approved these changes Mar 1, 2019

View reviewed changes

facebook-github-bot reviewed Mar 4, 2019

View reviewed changes

convert cuda round to half to even

6799da4

add cuda round test add a new blank line after test_cuda_round convert all cpu round to nearbyint

facebook-github-bot reviewed Mar 7, 2019

View reviewed changes

facebook-github-bot closed this in 8ec7357 Mar 7, 2019

pytorchbot added the merged label Mar 7, 2019

vishwakftw mentioned this pull request Mar 7, 2019

Different behavior of torch.round() on CPU and GPU #16498

Closed

musikisomorphie deleted the round branch March 7, 2019 08:52

gchanan mentioned this pull request Jun 17, 2019

Intoducing bfloat16 type #21522

Closed

ezyang added the open source label Jun 24, 2019

VitalyFedyunin mentioned this pull request Aug 22, 2019

Move the CUDA implementation of round to ATen. #25041

Closed

mirzadeh mentioned this pull request Jun 12, 2023

torch.round behaves inconsistently when running a traced model #103465

Closed

[WIP] fix different round behavior on CPU and GPU #16498 #17443

[WIP] fix different round behavior on CPU and GPU #16498 #17443

Uh oh!

Conversation

musikisomorphie commented Feb 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bhushan23 commented Feb 24, 2019

Uh oh!

soumith commented Feb 25, 2019

Uh oh!

vishwakftw commented Feb 25, 2019

Uh oh!

bhushan23 left a comment • 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!

musikisomorphie commented Feb 26, 2019

Uh oh!

musikisomorphie commented Feb 26, 2019

Uh oh!

vishwakftw commented Feb 26, 2019

Uh oh!

vishwakftw commented Feb 26, 2019

Uh oh!

musikisomorphie commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

musikisomorphie commented Feb 28, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

VitalyFedyunin commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

musikisomorphie commented Feb 28, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

VitalyFedyunin commented Mar 1, 2019

Uh oh!

vishwakftw commented Mar 1, 2019

Uh oh!

VitalyFedyunin commented Mar 1, 2019

Uh oh!

musikisomorphie commented Mar 1, 2019

Uh oh!

VitalyFedyunin commented Mar 1, 2019

Uh oh!

musikisomorphie commented Mar 1, 2019

Uh oh!

bhushan23 commented Mar 2, 2019

Uh oh!

musikisomorphie commented Mar 4, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

VitalyFedyunin commented Mar 4, 2019

Uh oh!

VitalyFedyunin commented Mar 4, 2019

Uh oh!

VitalyFedyunin commented Mar 5, 2019

Uh oh!

VitalyFedyunin commented Mar 6, 2019

Uh oh!

VitalyFedyunin commented Mar 6, 2019

Uh oh!

pytorchbot commented Mar 6, 2019

Uh oh!

musikisomorphie commented Mar 6, 2019

Uh oh!

VitalyFedyunin commented Mar 6, 2019

Uh oh!

musikisomorphie commented Mar 6, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

musikisomorphie commented Feb 24, 2019 •

edited

Loading

bhushan23 left a comment •

edited

Loading

musikisomorphie commented Feb 26, 2019 •

edited

Loading

VitalyFedyunin commented Feb 28, 2019 •

edited

Loading