This repository has been archived by the owner on Nov 17, 2023. It is now read-only.
float32 -> float16 cast consistency across implementations #13857
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mxnet-label-bot add [pr-awaiting-review] |
7 tasks
larroy
reviewed
Jan 17, 2019
| for numerator in range(0, denominator): | ||
| for y in [-1.0, 0.0, 1.0]: | ||
| small_delta = y / 2**fp32_fraction_bits | ||
| val = (-1.0)**sign_bit * 2.0**exponent * (1.0 + |
There was a problem hiding this comment.
nit: Could we break (1.0 + also in the next line for readability?
| # Test requires all platforms to round float32->float16 with same round-to-nearest-even policy. | ||
| @with_seed() | ||
| def test_cast_float32_to_float16(): | ||
| fp16_fraction_bits = 10 |
There was a problem hiding this comment.
shall we capitalize constants as per PEP8?
| sym_output = exe.outputs[0].asnumpy() | ||
| for fp32_val, model_fp16_val, np_fp16_val in zip(input_np, sym_output, expected_output): | ||
| if model_fp16_val != np_fp16_val: | ||
| raise RuntimeError('fp32->fp16 cast mismatches seen, e.g. with val {}, model_fp16 = {},' |
There was a problem hiding this comment.
Better raise assertionerror or use https://nose.readthedocs.io/en/latest/testing_tools.html as RuntimeError has a different semantics
|
@larroy - Can you please take a look back at this PR? Your comments are addressed. |
|
Now that a dependent mshadow PR has been merged, I will be updating the mshadow SHA used by this PR shortly, after which this PR will be ready for merging. Should be an easy approval process as this PR only introduces a test. |
|
@larroy This PR is ready for your final review. |
|
Sorry, having a look now. |
|
@mxnet-label-bot update [Operator, pr-awaiting-merge] |
marcoabreu
added
pr-awaiting-merge
stephenrawls
pushed a commit
to stephenrawls/incubator-mxnet
that referenced
this pull request
Feb 16, 2019
) * Added test showing float32->float16 discrepancy when mshadow float2half() is used. * Temp update mshadow submodule SHA to point to PR368 (b211cb7). * Temp switch to url = https://github.com/DickJC123/mshadow.git * Updata mshadow submodule SHA. * Improve code style per reviewer comments. * Move back to dmlc/mshadow.git, now with float->half rounding. * Expand test_operator.py:test_cast_float32_to_float16 to test np.nan.
vdantu
pushed a commit
to vdantu/incubator-mxnet
that referenced
this pull request
Mar 31, 2019
) * Added test showing float32->float16 discrepancy when mshadow float2half() is used. * Temp update mshadow submodule SHA to point to PR368 (b211cb7). * Temp switch to url = https://github.com/DickJC123/mshadow.git * Updata mshadow submodule SHA. * Improve code style per reviewer comments. * Move back to dmlc/mshadow.git, now with float->half rounding. * Expand test_operator.py:test_cast_float32_to_float16 to test np.nan.
haohuanw
pushed a commit
to haohuanw/incubator-mxnet
that referenced
this pull request
Jun 23, 2019
) * Added test showing float32->float16 discrepancy when mshadow float2half() is used. * Temp update mshadow submodule SHA to point to PR368 (b211cb7). * Temp switch to url = https://github.com/DickJC123/mshadow.git * Updata mshadow submodule SHA. * Improve code style per reviewer comments. * Move back to dmlc/mshadow.git, now with float->half rounding. * Expand test_operator.py:test_cast_float32_to_float16 to test np.nan.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
While trying to get all the CI runners to pass for PR #13749, I discovered that the handling of the float32->float16 cast on the CPU varies based on whether the f16c library is available and enabled. If the f16c library is not available, as is the case for the Windows CI runner using a MSVC++ compiler, then the mshadow float2half() routine is used and the cast is performed by truncating the bits that don't fit in the float16 representation. The _cvtss_sh(data, 0) call employed by mshadow when the f16c library is present performs a round-to-nearest conversion, with ties rounded to the value with a 0 LSB. This round-to-nearest-even policy also is employed by the default GPU context implementation and numpy.
In order to improve MXNet model and CI consistency across all backends, I'm correcting the mshadow float2half() implementation to perform matching round-to-nearest-even rounding. The first commit introduces only a test that demonstrates the problem, which I am expecting to fail on the Windows CI runner. Then I'll work up an mshadow PR with the new float2half() routine. Next, I'll change the mshadow commit used by MXNet to point to this PR, to demonstrate its effectiveness. Once the mshadow PR has been accepted, I'll change the mshadow commit used by MXNet once again so the PR can be accepted.
This PR should only effect models run without the f16c library on a CPU. I intend to make the round-to-nearest-even behavior the new default for these scenarios, bringing them in line with other systems. I'll provide a simple build flag to restore the legacy behavior. My new float2half() implementation is 50% faster on the cpu despite the additional rounding logic.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments