Skip to content

Clean up arglist for choose_qparams_affine_with_min_max#3808

Merged
jerryzh168 merged 4 commits into
pytorch:mainfrom
mohammed-saalim:cleanup-choose-qparams-affine-with-min-max
Feb 6, 2026
Merged

Clean up arglist for choose_qparams_affine_with_min_max#3808
jerryzh168 merged 4 commits into
pytorch:mainfrom
mohammed-saalim:cleanup-choose-qparams-affine-with-min-max

Conversation

@mohammed-saalim

@mohammed-saalim mohammed-saalim commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #3747
This PR removes preserve_zero and zero_point_domain parameters from choose_qparams_affine_with_min_max, keeping only the code path for preserve_zero=True and zero_point_domain=ZeroPointDomain.INT, similar to other cleanups in the file (using _choose_qparams_affine as reference).

Changes

  • Removed preserve_zero parameter (always True now)
  • Removed zero_point_domain parameter (always INT now)
  • Simplified 57 lines down to 22 lines
  • Updated callers in observer.py

@mohammed-saalim
Clean up arglist for choose_qparams_affine_with_min_max
b1cf1f4 
Remove preserve_zero and zero_point_domain parameters from
choose_qparams_affine_with_min_max and keep only the code path
for preserve_zero=True and zero_point_domain=ZeroPointDomain.INT,
similar to other cleanups in the file.

Fixes pytorch#3747
@pytorch-bot

pytorch-bot Bot commented Feb 3, 2026 •
edited
Loading

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/3808

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

⏳ No Failures, 2 Pending

As of commit dec9c9b with merge base a996516 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 3, 2026
@jerryzh168

jerryzh168 commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

thanks, can you check if the tests and all users of AffineQuantizedMinMaxObserver are still running? https://github.com/search?q=repo%3Apytorch%2Fao%20AffineQuantizedMinMaxObserver&type=code

e.g. tutorials/calibration_flow/awq_like.py

tutorials/calibration_flow/gptq_like.py

tutorials/calibration_flow/static_quant.py

ao/test/quantization/test_observer.py

Line 43 in 0a5638a

def test_min_max_per_tensor_affine(self):

@mohammed-saalim
Handle ZeroPointDomain.NONE in observers after qparams cleanup
408fc9a 
The cleaned-up choose_qparams_affine_with_min_max now always returns
a tensor zero_point, so observers that use ZeroPointDomain.NONE
(e.g., for float8 symmetric quantization) need to post-process
the result to return None for zero_point.

This maintains backward compatibility for:
- AffineQuantizedMinMaxObserver.calculate_qparams()
- AffineQuantizedMSEObserver.loss_fn()
- AffineQuantizedMSEObserver.calculate_qparams()

Refs pytorch#3747
@mohammed-saalim

mohammed-saalim commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @jerryzh168!

I checked the files you mentioned:

Tutorials work correctly:

tutorials/calibration_flow/awq_like.py - Uses AffineQuantizedMinMaxObserver without preserve_zero or zero_point_domain
tutorials/calibration_flow/gptq_like.py - Same
tutorials/calibration_flow/static_quant.py - Same
Tests fixed: I found that test_observer.py uses ZeroPointDomain.NONE in several tests for float8 symmetric quantization. These tests expect zero_point=None to be returned.

I pushed a fix that handles ZeroPointDomain.NONE in the observers by setting zero_point=None after calling the simplified choose_qparams_affine_with_min_max. This maintains backward compatibility while keeping the function clean.

The observers still accept zero_point_domain in their constructors, but choose_qparams_affine_with_min_max no longer needs those parameters.

@jerryzh168

jerryzh168 commented Feb 4, 2026

Copy link
Copy Markdown
Contributor

The observers still accept zero_point_domain in their constructors, but choose_qparams_affine_with_min_max no longer needs those parameters.

I think we probably need to add some assert to make sure the zero_point_domain==INT and preserve_zero=True as well? (that's the path that's kept), also print a warning to say these args are deprecated would be good

also thanks for checking all these tests

jerryzh168
jerryzh168 reviewed Feb 4, 2026
View reviewed changes
Comment thread torchao/quantization/observer.py
@mohammed-saalim
Address reviewer feedback: deprecation warnings and remove NONE handling
202732b 
Per @jerryzh168 review:
1. Add deprecation warnings in AffineQuantizedObserverBase.__init__ when
   preserve_zero != True or zero_point_domain != ZeroPointDomain.INT
2. Remove ZeroPointDomain.NONE handling from observer methods - callers
   should use 'scale, _ = calculate_qparams()' to ignore zero_point
3. Update tests to remove ZeroPointDomain.NONE usage and ignore zero_point
   where not needed (symmetric quantization)

Refs pytorch#3747
@mohammed-saalim

mohammed-saalim commented Feb 4, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a commit addressing both points:

  1. Added deprecation warnings in AffineQuantizedObserverBase.__init__ for non-default preserve_zero and zero_point_domain values
  2. Removed NONE handling from observers - tests now use scale, _ = calculate_qparams() to ignore zero_point
  3. Updated all test callsites to remove ZeroPointDomain.NONE usage
    Thanks for the review! @jerryzh168

@mohammed-saalim

mohammed-saalim commented Feb 5, 2026

Copy link
Copy Markdown
Contributor Author

Hi @jerryzh168
Is there any changes which needs to be made? I would be happy to work on it. Thanks

jerryzh168
jerryzh168 reviewed Feb 5, 2026
View reviewed changes
Comment thread torchao/quantization/quant_primitives.py Outdated
Mostly same as :func:`~torchao.quantization.quant_primitives.choose_qparams_affine`. with one
difference: instead of passing in `input` Tensor and use that to calculate min_val/max_val
and then scale/zero_point, we pass in min_val/max_val directly
min_val (torch.Tensor): minimum values derived from calibration data

@jerryzh168 jerryzh168 Feb 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: might be better to revert this change so we don't duplicate docs I think

jerryzh168
jerryzh168 reviewed Feb 5, 2026
View reviewed changes

@jerryzh168 jerryzh168 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for the fix! please revert the doc change for choose_qparams_affine_with_min_max to not duplicate the docs

@mohammed-saalim
Address reviewer feedback and fix CI failures
dec9c9b 
- Revert documentation changes in choose_qparams_affine_with_min_max to avoid duplication
- Restore ZeroPointDomain.NONE handling in observers for float8 symmetric quantization
- Add zero_point_domain=ZeroPointDomain.NONE to TestLinearObserver tests
- Fix unused import warning and test failures

Refs pytorch#3747
@mohammed-saalim

mohammed-saalim commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

@jerryzh168! 👋

Thanks for the review! I've addressed all the feedback and fixed the CI failures:

Changes Made:

  1. Reverted documentation changes in choose_qparams_affine_with_min_max to avoid duplication (as you requested)
  2. Fixed the Ruff linter error - Added zero_point_domain=ZeroPointDomain.NONE to the TestLinearObserver tests, so the import is now used
  3. Fixed test failures - Restored ZeroPointDomain.NONE handling in the observer methods to return zero_point=None for float8 symmetric quantization
  4. Code formatting - Ran ruff format on all modified files

What Changed:
The deprecation warnings you suggested remain intact. The ZeroPointDomain.NONE handling is now at the observer level - choose_qparams_affine_with_min_max always returns an integer zero_point (keeping it simple), and the observers post-process it to None when zero_point_domain == ZeroPointDomain.NONE.

All local linter checks pass:

  • ruff check --isolated --select F821,F823,W191
  • ruff check
  • ruff format --check

The CI checks don't seem to have started yet. Could you help trigger a rerun? Thanks!

@mohammed-saalim

mohammed-saalim commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Could you please add the appropriate PR label? The "PR Label Check" was one of the CI failures. Thanks!

@jerryzh168 jerryzh168 added the topic: improvement Use this tag if this PR is an improvement (doesn't fit into any of the other categories) label Feb 6, 2026
@jerryzh168

jerryzh168 commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

@mohammed-saalim I added the label, but want to check, do you also have permission to add labels?

@mohammed-saalim

mohammed-saalim commented Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @jerryzh168!

All checks have passed now!

Regarding label permissions - no, I don't have permission to add labels. As a first-time contributor to the repo, I only have read/write access to my fork.

@jerryzh168 jerryzh168 merged commit fc4caf2 into pytorch:main Feb 6, 2026
19 of 20 checks passed
@jerryzh168 jerryzh168 changed the title Clean up arglist for choose_qparams_affine_with_min_max Clean up arglist for choose_qparams_affine_with_min_max Feb 6, 2026
@jerryzh168 jerryzh168 added module: not user facing Use this tag if you don't want this PR to show up in release notes module: core changes affecting multiple modules, e.g. base config/tensor, observers, quant ops and removed module: not user facing Use this tag if you don't want this PR to show up in release notes labels Feb 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jerryzh168 jerryzh168 jerryzh168 approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: core changes affecting multiple modules, e.g. base config/tensor, observers, quant ops topic: improvement Use this tag if this PR is an improvement (doesn't fit into any of the other categories)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clean up arglist for choose_qparams_affine_with_min_max

2 participants

@mohammed-saalim @jerryzh168