Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rounding on float16 conversions #8378

Merged
merged 1 commit into from
Nov 7, 2019
Merged

Conversation

emcastillo
Copy link
Member

@emcastillo emcastillo commented Nov 5, 2019

Closes #8366

When casting a Chainerx array in the native backend to float16 the float16 code differs subtly from the numpy one in the rounding of elements.

https://github.com/numpy/numpy/blob/1f8fb2ca31a7f41a23c60e4ffcb38b3e0cb201f7/numpy/core/src/npymath/halffloat.c#L339-L347

This caused the IRemainder test to yield 150 failures for 1 million runs.
After fixing the number went down to 0.

The fp64 to fp16 case has been similarly fixed to match the numpy logic as some of the calculations were different and it was not clear where did they come from.

chainerx_cc/chainerx/float16.cc Outdated Show resolved Hide resolved
chainerx_cc/chainerx/float16.cc Outdated Show resolved Hide resolved
chainerx_cc/chainerx/float16.cc Outdated Show resolved Hide resolved
@niboshi
Copy link
Member

niboshi commented Nov 5, 2019

Also please write tests.

@emcastillo
Copy link
Member Author

I added an initial try for testing it, it is for the float32 case,
If its ok I will extend it to take the float64 in account too.

Since this is a numpy related change I was doubting on how to do the testing.
The two alternatives that I though was to explicitly test if a number was not rounded, or
add a new python test which compares chainerx and numpy type castings. I went for the first one since the scope seems broader and the later its extensively tested in the routines.

@asi1024
Copy link
Member

asi1024 commented Nov 7, 2019

Jenkins, test this please.

@asi1024 asi1024 added this to the v7.0.0 milestone Nov 7, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 51e1ec4, target branch master) succeeded!

@asi1024
Copy link
Member

asi1024 commented Nov 7, 2019

LGTM.

@asi1024 asi1024 merged commit c8da0d2 into chainer:master Nov 7, 2019
@emcastillo emcastillo changed the title Fix rounding on float16 conversions Fix rounding on float16 conversions Dec 5, 2019
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 51e1ec4, target branch master) failed with status FAILURE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:bug Bug report or fix.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky test: chainerx_tests/unit_tests/routines_tests/test_arithmetic.py::test_IRemainder
4 participants