Add fp8_e8m0 scale type support - #99
Conversation
Code reviewSelf-review notes from re-reading the branch against OCP MX v1.0: Confirmed the design direction
Small cleanups worth doing before merge
Out of scope (deferred, deliberately)
|
Fixes:
|
Code Review1. When a group's 2. The new early-return 3. Two test cases are silently dropped from The "500.0 overflows to NaN in e4m3" test (lines 829–831) computes 4. Lean 4's 5. There is no validation of 6. Stale contradictory comment on the scaling direction in Line 846 says 7. The comment says "change from private since I need to call this in another file for quantizing" — but |
…vate, and fix comments
Fixes:
|
Cleanup / SimplifyFix 1 — Replace: let absV := if v < 0.0 then -v else v
if absV > acc then absV else acc) 0.0With: if v.abs > acc then v.abs else acc) 0.0
Fix 2 —
let scaledVals <- group.mapM fun elemBytes => do
let v <- Dtype.byteArrayToFloat32 .float32 elemBytes
...With: let scaledVals <- vals.mapM fun v =>
Dtype.byteArrayOfFloat32 .float32 (v * m)Fix 3 —
x.shape.val.dropLast ++ [x.shape.val.getLast?.getD 0 / groupSize]With: x.shape.val.dropLast ++ [lastDim / groupSize]Fix 4 —
let ratio := fp8Max / amax
if ratio.isInf then (scaleByte = 254, m = Float32.ofBits 0x7F000000)
else
let logM := ratio.log2.floor
-- scaleByte uses: (-logM + 127.0).toUInt8
-- m uses: Float32.pow 2.0 logM |
Fixes:
|
Code Review (updated PR)Most findings from the previous review have been addressed — good work on the fixes. One confirmed bug and one gap remain. 1. The branch: else if ratio.isInf then (254, Float32.ofBits 0x7F000000)The multiplier The E8M0 spec stores else if ratio.isInf then (0, Float32.ofBits 0x7F000000)
2. No test covers the This branch fires when -- subnormal amax: ratio overflows to inf, should use max scale (byte 0 = 2^-127)
let x <- IO.ofExcept (Tensor.ofFloat32List [Float32.ofBits 0x00000001]) -- smallest subnormal
let (qW, scales) <- IO.ofExcept (Tensor.quantizeMX x 1 .float8_e4m3)
let pass := scales.data == ByteArray.mk #[0] -- scale byte 0 = 2^-127 |
Fixes:
|
Code Review (updated PR)The 1. The vals.foldl (fun acc v => if v.abs > acc then v.abs else acc) 0.0never produces NaN. In IEEE 754, A consequence: the Fix: detect NaN groups before the fold: let hasNaN := vals.any Float32.isNaN
let amax := if hasNaN then Float32.nan
else vals.foldl (fun acc v => if v.abs > acc then v.abs else acc) 0.0Then the existing 2. Typo in error message — "quantizeMX: inpute tensor must be float32"should be |
…-checked) and fix spelling error
Fixes:
|
Code Review (updated PR)All previous findings are fixed. The NaN detection with One gap remains: Missing test:
-- NaN input: group with NaN should emit scale byte 255 (OCP NaN sentinel)
let xNaN <- IO.ofExcept (Tensor.ofFloat32List [Float32.ofBits 0x7FC00000, 1.0]) -- NaN, 1.0
let (_, scalesNaN) <- IO.ofExcept (Tensor.quantizeMX xNaN 2 .float8_e4m3)
let passNaN := scalesNaN.data == ByteArray.mk #[255]
IO.println s!"quantizeMX NaN input group (scale byte 255): {passNaN}"
checks := passNaN :: checks |
Fixes:
|
Code Review (updated PR)The NaN test is now in place and wired into LGTM — no new issues found. |
Fp8_e8m0 is an unsigned 8-bit type where all bits are exponent (bias=127, no mantissa, no sign).
Type system
Decoder
Note: Implemented as per OCP MX spec
Testing
Note: I gave not implemented an encoder since acc to OCP, producing an E8M0 byte is a block-level operation (compute group max, derive scale), not a per-value cast. Encoder will be added when block quantization is implemented.