Skip to content

Add small repro test for unsigned -> signed et loss error #8506

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

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

dulinriley
Copy link
Contributor

@dulinriley dulinriley commented Feb 14, 2025

Summary:
There was a difference in behavior from quantized_decomposed.quantize_per_tensor
and cadence.quantize_per_tensor, specifically how rounding half values worked.
The former rounds towards even (based on torch.round which does that).
The latter rounds away from zero.

Make sure the python implementation matches the Executorch implementation in this
regard.

Differential Revision: D69668881

Copy link

pytorch-bot bot commented Feb 14, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8506

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures, 1 Unrelated Failure

As of commit 13a997c with merge base c00c798 (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Feb 14, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D69668881

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D69668881

@dulinriley
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

dulinriley added a commit to dulinriley/executorch that referenced this pull request Feb 18, 2025
Summary:

There was a difference in behavior from `quantized_decomposed.quantize_per_tensor`
and `cadence.quantize_per_tensor`, specifically how rounding half values worked.
The former rounds towards even (based on `torch.round` which does that).
The latter rounds away from zero.

Make sure the python implementation matches the Executorch implementation in this
regard.

Reviewed By: sabarishsnk

Differential Revision: D69668881
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D69668881

Summary:

There was a difference in behavior from `quantized_decomposed.quantize_per_tensor`
and `cadence.quantize_per_tensor`, specifically how rounding half values worked.
The former rounds towards even (based on `torch.round` which does that).
The latter rounds away from zero.

Make sure the python implementation matches the Executorch implementation in this
regard.

Reviewed By: sabarishsnk

Differential Revision: D69668881
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D69668881

@facebook-github-bot facebook-github-bot merged commit f174ca8 into pytorch:main Feb 19, 2025
43 of 48 checks passed
@@ -16,6 +16,9 @@
from numbers import Number
from typing import cast, Sequence

# Import these for the cadence function signatures.
import executorch.backends.cadence.aot.ops_registrations # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

This line broke the linter job: https://github.com/pytorch/executorch/actions/runs/13398623818/job/37423762070#step:15:7593

I think it needs two spaces before the #. Please run lintrunner -a to fix this, and make sure that the job is passing before merging PRs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sent #8575 to fix this

dbort added a commit that referenced this pull request Feb 19, 2025
Fix lint issue in backends/cadence/aot/fuse_ops.py introduced in #8506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants