Skip to content

fix(graph): correct shift handling in quantize_float / dequantize roundtrip#1027

Open
abhicris wants to merge 1 commit into
zkonduit:mainfrom
abhicris:fix/quantize-dequantize-shift-roundtrip
Open

fix(graph): correct shift handling in quantize_float / dequantize roundtrip#1027
abhicris wants to merge 1 commit into
zkonduit:mainfrom
abhicris:fix/quantize-dequantize-shift-roundtrip

Conversation

@abhicris
Copy link
Copy Markdown

Summary

quantize_float(elem, shift, scale) and dequantize(felt, scale, shift) in
src/graph/utilities.rs are documented as inverses, but their formulas only
agree when shift == 0.

  • quantize_float computes q = round(elem * 2^scale + shift).
  • dequantize computes elem ≈ q / 2^scale - shift, which is the inverse of
    q = (elem + shift) * 2^scale — a different fixed-point convention.

So for any caller that passes a non-zero shift, dequantize silently returns
wrong values. The bug is dormant for in-tree callers today (every internal use
of dequantize passes shift = 0.0 — see src/graph/mod.rs:188,204), but
dequantize is pub, the shift parameter is part of its signature, and the
docstring promises it works for the general case.

While in this region I also fixed an asymmetric-range bug in quantize_float's
bound check. The valid range of elem is

(IntegerRep::MIN - shift) / mult  <=  elem  <=  (IntegerRep::MAX - shift) / mult

but the code used a symmetric [-max_value, max_value] check, which is correct
only when shift == 0. For non-zero shift it both over- and under-rejects by
shift / mult on each side. Now uses the correct asymmetric bounds.

Why this bug exists

The two functions were almost certainly written assuming shift = 0 (which is
the only path internally exercised today), and the asymmetric shift semantics
were never re-derived when shift was added to the public API. There is no
round-trip test in the repo that exercises non-zero shift, which is how the
inconsistency stayed undetected.

What the fix does

  1. dequantize now computes (int_rep - shift) / multiplier, which is the
    true mathematical inverse of q = elem * mult + shift.
  2. quantize_float now computes both max_value and min_value from the
    actual IntegerRep::MAX/IntegerRep::MIN bounds and uses them in the
    range check.
  3. Docstrings updated to explicitly state the quantize/dequantize formulas so
    the invariant is harder to forget next time.
  4. Four new tests in src/graph/utilities.rs:
    • test_quantize_dequantize_roundtrip_zero_shift — baseline; ensures the
      common shift = 0 path still roundtrips (would have stayed green with
      either formula).
    • test_quantize_dequantize_roundtrip_with_shift — exercises the fix; this
      test fails on master and passes here.
    • test_quantize_dequantize_known_value_with_shift — hand-checked numeric
      case (scale = 4, shift = 5.0, x = 2.0q = 37 → recovered 2.0).
      Pre-fix, dequantize returned -2.6875 for the same inputs.
    • test_quantize_float_asymmetric_bounds_with_shift — covers the bound-check
      correction.

Validation

Tested with the repo's pinned nightly-2025-12-01 toolchain on aarch64-darwin:

$ cargo check
    Finished `dev` profile [unoptimized + debuginfo] target(s)

$ cargo test --lib graph::utilities::tests::test_quantize
running 6 tests
test graph::utilities::tests::test_quantize_edge_cases ... ok
test graph::utilities::tests::test_quantize_float_asymmetric_bounds_with_shift ... ok
test graph::utilities::tests::test_quantize_dequantize_known_value_with_shift ... ok
test graph::utilities::tests::test_quantize_dequantize_roundtrip_zero_shift ... ok
test graph::utilities::tests::test_quantize_dequantize_roundtrip_with_shift ... ok
test graph::utilities::tests::test_quantize_tensor ... ok
test result: ok. 6 passed; 0 failed

$ cargo test --lib graph::
test result: ok. 10 passed; 0 failed; 0 ignored

$ cargo test --lib fieldutils::
test result: ok. 4 passed; 0 failed; 0 ignored

Behaviour-change risk

Internal callers all pass shift = 0.0, where the new and old formulas
coincide bit-for-bit. No in-tree behaviour changes.

External callers that were passing non-zero shift to dequantize will
start receiving correct values instead of the previous incorrect ones. That
is the intended fix, but worth flagging in the changelog.

Out of scope

  • quantize_float's handling of f64::NAN (it relies on the f64 as i128
    saturating cast returning 0, as the existing test_quantize_edge_cases
    test pins). Not changed.
  • Overflow behaviour at the absolute i128::MAX boundary, where
    IntegerRep::MAX as f64 already loses precision. Not changed.
  • Surrounding panics in src/graph/mod.rs flagged by chore(graph): remove panics in input conversion and batching checks #1011. Orthogonal.

Test plan

  • cargo check — green
  • cargo test --lib graph::utilities::tests::test_quantize — 6 passed
  • cargo test --lib graph:: — 10 passed
  • cargo test --lib fieldutils:: — 4 passed

…ndtrip

`quantize_float(elem, shift, scale)` defines `q = round(elem * 2^scale + shift)`,
but `dequantize(felt, scale, shift)` previously computed `q / 2^scale - shift`,
which is the inverse of `q = (elem + shift) * 2^scale` instead. The two
formulas agree only when `shift == 0` (the path used internally today), so the
bug was dormant for in-tree callers but `dequantize` is a public function and
silently returns wrong values for any external caller that passes a non-zero
shift.

The correct inverse is `(q - shift) / 2^scale`. With this change,
`dequantize(quantize_float(x, s, scale), scale, s) ≈ x` for every `s`, not
only `s = 0`.

While in this region I also fixed a related bound in `quantize_float`. The
representable range of `elem` is asymmetric whenever `shift != 0`:

    (IntegerRep::MIN - shift) / mult  <=  elem  <=  (IntegerRep::MAX - shift) / mult

The old check used a symmetric bound `[-max_value, max_value]`, which under-
or over-rejected by `shift / mult` on each side. Replaced with the correct
asymmetric bounds.

Tests added in `src/graph/utilities.rs`:

  - test_quantize_dequantize_roundtrip_zero_shift   (regression-safe baseline)
  - test_quantize_dequantize_roundtrip_with_shift   (exercises the fix)
  - test_quantize_dequantize_known_value_with_shift (hand-checked numeric case)
  - test_quantize_float_asymmetric_bounds_with_shift (bound-check correctness)

Out of scope: behaviour of `quantize_float` on `f64::NAN` (still relies on the
`f64 as i128` saturating cast returning 0, as the existing test documents);
overflow behaviour at the absolute `i128::MAX` boundary, where the `as f64`
cast already loses precision. Both are pre-existing and orthogonal to the
shift-roundtrip bug fixed here.

Validated locally with the project's pinned nightly toolchain:

  cargo check                                          -> ok
  cargo test --lib graph::utilities::tests::test_quantize -> 6 passed, 0 failed
  cargo test --lib graph::                                -> 10 passed, 0 failed
  cargo test --lib fieldutils::                           -> 4 passed, 0 failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant