🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

❌ 2 New Failures

As of commit beab4c1 with merge base 7963f9c ():

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

Please don't merge yet, this isn't good enough...

Ok, I think it could be reviewed now. Basically, in calculate_scale_eps_for_dtype(), scale calculations are kind of emulated, and the minimum value that won't produce an Inf when reciprocated is returned. This should produce such eps value for choose_qparams_affine() that would calculate the scale so that the range of quantized values is maximized, but that scale reciprocal, used when given tensor actually quantized, doesn't become Inf.

Now, this is all probably an overkill: it's pretty much relevant only for float16 inputs, it could be that it fixes only one of several quantization code paths, etc. So maybe I just put this in my #1671 for now, specifically for the quantization type where I've encountered the issue while I was testing it?

Closing as the issue is rather improbable to encounter in practice.

By the way, thanks for fixing this!

I think this PR should include a test case which fails before and passes after these changes.

I think this PR should include a test case which fails before and passes after these changes.

Added a test case - without changes in torchao/quantization/quant_primitives.py, it will produce Inf scale for all "high-precision" floating point data types tested. (I've put some comments in the test code that I hope explain the issue.)

I see you point about clamping twice. I need to see if some further changes in torchao/quantization/quant_primitives.py are needed anyway. The problem is that, it seems to me, the scale could end up calculated in data type different from scale_data type, that it will be eventually casted to, and also that for asymmetric mapping, its reciprocal actually gets used immediately - and it has to be properly clamped from below in all the cases. Thus, I don't think it is possible just to fix eps before branching on mapping type.

(The eps argument probably should hot have been there to start with - if we're not completely sure how to choose it, the users are even less.)

Pushed an update, I think this is it. Namely, in _choose_qparams_affine():

1. For floating point inputs: scale is calculated in min_val/max_val dtype, so eps is clamped against the smallest normalized value of this datatype, to have scale clamped againt this eps value, and thus prevent scale reciprocal, used here to become Inf.
2. For integer inputs, scale ends up calculated as torch.float32 tensor (because min_val/max_val, that is an integer tensor, is part of arithmetic operations with a Python float value, and it seems the results in such case is promoted to torch.float32), so eps is clamped to smallest normalized value of torch.float32 - the clamping is for the same reason as in the previous case.
3. At the end of the function, scale is converted to scale_dtype dtype, so if this dtype is floating point, then before returning the value, it is clamped against the smallest normalized value of this datatype, again to prevent scale reciprocal (now if used from call site), to become Inf.