Add float8_e4m3fn dtype support#95
Conversation
|
Code review findings on this PR: 1.
|
…tFloatUnop, remove dead branch, and update fp8 prefix in test case prints
Fixes:
|
Self-review at max effort — float8_e4m3 branchSix findings, most-severe first: 1.
|
Fixes:
|
Code-review findingsTensorLib/Dtype.lean:141 — the Because |
|
Removed dead arm in join. |
seanmcl
left a comment
There was a problem hiding this comment.
Thanks for this — the core fp8 conversion is really solid. I verified the encoder/decoder against ml_dtypes.float8_e4m3fn and found 0 mismatches across all 256 decode bytes, ~2M random fp32 encodes, all 17.6k rounding-tie midpoints, and all 65,536 operand pairs for add/sub/mul/div. Every #guard/test constant checks out, the join promotion table matches np.result_type, and castOverflow is exhaustive. Nice work on the round-to-nearest-even handling.
The issues I found are all at the integration edges, not the bit math. Inline comments below; one that can't be inlined (the file isn't in this diff):
🔴 Tensor.lean:602 — toFloat32Tree/toFloat64Tree don't handle fp8. These dispatch fp16/bf16 explicitly (lines 600-601) but route everything else through | _ => Float32.ofLEByteArray b / Float.ofLEByteArray b, which require exactly 4/8 bytes. On a 1-byte fp8 element the Err variant returns "Expected size 4 byte array" for every element, and the ! variant panics. fp8 tensors can't be converted/formatted to float trees at all — a functional gap vs fp16/bf16. Worth adding a .float8_e4m3 arm (this mirrors the same class of miss as adding an enum case without widening a _ dispatch).
…uards, double-round comment
|
Also added .float8_e4m3 (used helper defined in dtype.lean) arm to toFloat32Tree and toFloat64Tree in Tensor. Its not a private function since I need to access it in tensor.lean. |
|
|
ml_dtypes and numpy raise dtype promotion error with fp8_e4m3 , float16/ bfloat16 which matches current lean implementation so no changes. |
|
Reviewed the fp8_e4m3 diff against Three lower-severity items:
|
Fixes:
|
seanmcl
left a comment
There was a problem hiding this comment.
Re-reviewed at the new head (fa72f9e) — thanks for the quick turnaround. I confirmed all six items from the last pass are correctly addressed, and the changes build clean with all #guards passing:
- ✅
Tensor.lean—toFloat32Tree/toFloat64Treenow have explicit.float8_e4m3arms. I traced the leaf path (toByteArrayTree→getPositionslicesitemsize= 1 byte for fp8), sodecodeFloat8E4M3's size guard passes and fp8 tensors decode correctly. TheFloat32.toFloatwidening intoFloat64Treeis exact. - ✅
decodeFloat8E4M3/encodeFloat8E4M3helpers — all former inline sites converted, none missed. I verifiedencodeFloat8E4M3 f = ByteArray.mk #[f.toFloat8E4M3Bits]is byte-identical to the previous forms (theToLEByteArray UInt8instance isByteArray.mk #[n]), so this is a pure extraction with no behavior change. Re-ran the numeric fuzz againstml_dtypesafter the dead-code removal: still 0 mismatches over 3M+ values. - ✅
abs/isZero— the removed inlinesize != 1guards are fully replaced by the helper's guard; the.isOk == false#guards still hold. - ✅
Float.lean— the deadfinalExp == 15 && finalMant == 7branch is gone (I re-confirmed it's genuinely unreachable), and both comments (line 227 NaN semantics, line 251 NaN sign) are now accurate. - ✅
Npy.lean— the<V1/|V1collision#guards are present and pass, matching the bf16V2precedent.
Nothing blocking from me. One tiny consistency nit inline; the pre-existing float64→fp8 double-rounding is now documented, which is fine. LGTM.
Fp8_e4m3 = 1 sign, 4 exp, 3 mantissa (bias = 7, max + +-448, no inf, NaN only)