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

Avoid loss of precision from using reciprocal #55310

Merged
merged 1 commit into from Mar 28, 2022

Conversation

elfringham
Copy link
Contributor

Taking the reciprocal of the calculated value results in a loss of precision. This causes the unit test prepare-tf.mlir.test to fail on AARCH64. So instead of taking the reciprocal of the calculated nudged_scale to get the inv_nudged_scale, calculate this value from the input values.

@google-ml-butler google-ml-butler bot added the size:XS CL Change Size: Extra Small label Mar 21, 2022
@elfringham
Copy link
Contributor Author

@cfRod @nSircombe

@gbaned gbaned added the comp:core issues related to core part of tensorflow label Mar 22, 2022
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Mar 22, 2022
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 22, 2022
@cfRod
Copy link
Contributor

cfRod commented Mar 22, 2022

Adding @penpornk

Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

@@ -88,7 +88,7 @@ struct FakeQuantWithMinMaxArgsFunctor {
Nudge(min, max, quant_min, quant_max, &nudged_min, &nudged_max,
&nudged_scale);

const float inv_nudged_scale = 1.0f / nudged_scale;
const float inv_nudged_scale = (quant_max - quant_min) / (max - min);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computing this here risks being out of sync with nudge_scale if its computation is updated in the future. Please modify the Nudge function to also return inv_nudge_scale instead. Please also add a comment there that inv_nudge_scale is computed separately to preserve precision.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Mar 22, 2022
@penpornk penpornk removed the awaiting review Pull request awaiting review label Mar 22, 2022
@elfringham
Copy link
Contributor Author

@penpornk Updated as requested.

@gbaned gbaned requested a review from penpornk March 23, 2022 03:55
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Mar 23, 2022
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes!

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 25, 2022
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 25, 2022
@penpornk penpornk removed the awaiting review Pull request awaiting review label Mar 25, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 25, 2022
@copybara-service copybara-service bot merged commit 8e59d46 into tensorflow:master Mar 28, 2022
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 28, 2022
copybara-service bot pushed a commit that referenced this pull request Mar 28, 2022
PiperOrigin-RevId: 437683252
@elfringham
Copy link
Contributor Author

@penpornk What happened here? Why the rollback?

@penpornk
Copy link
Member

@elfringham It broke some internal tests which use fixed golden values. I'll do a more thorough test before bringing this PR back. You don't need to do anything.

copybara-service bot pushed a commit that referenced this pull request Apr 7, 2022
Imported from GitHub PR #55310

Taking the reciprocal of the calculated value results in a loss of precision. This causes the unit test prepare-tf.mlir.test to fail on AARCH64. So instead of taking the reciprocal of the calculated nudged_scale to get the inv_nudged_scale, calculate this value from the input values.
Copybara import of the project:

--
a80736f by Andrew Goodbody <andrew.goodbody@linaro.org>:

Avoid loss of precision from using reciprocal

PiperOrigin-RevId: 440188576
@elfringham elfringham deleted the prepare_mlir_fix branch January 26, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:core issues related to core part of tensorflow size:XS CL Change Size: Extra Small
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

5 participants