Bfloat16#94
Conversation
Code review — bfloat16Reviewed at high effort. Every numeric/logic claim below was verified empirically: I ran the exact functions in Lean 4.30.0 and cross-checked bit patterns and promotion/cast tables against 🔴 Correctness1. Inserting | .uint16, .bfloat16 => nonebetween
This flows into 2. bf16 exactly represents every value in 3. bf16 npy dtype only round-trips at little-endian —
🟡 Correctness (doc)4. NaN-sign comment is inaccurate — The comment says dropping the NaN sign bit "matches ml_dtype's bfloat16 constructor behavior." It doesn't: 🟡 Test quality5. Weak / missing arithmetic + cast tests —
6. Round-trip PBT NaN escape hatch —
7. Max-value assertion too weak —
🟢 Cleanup8. 9. New decode/encode helpers not reused — 10. Duplicated comment + typo — Also checked and refuted (no action needed): |
Correctness:
Correctness (doc):
Test Quality:
Cleanup:
|
…ne, add int8 -> bf16 lossless, add NaN comment, add arithmetic and cast tests, refactorfloatVariant, isZero, and castOverflow to use helper functions
Code Review FindingsBug:
|
|
Fixes for PR:
|
Re-review after fix commit (905743c)All previously flagged issues have been addressed. The branch is clean:
No new issues found. LGTM. |
|
Reviewed the revision (max-effort pass: 10 finder angles + a verification pass, with the numeric/
One new correctness regression
Pre-existing, surfaced by this change (not blocking, but the PR touches the surface)
Cleanup / robustness (optional)
Verified and not an issueI checked and cleared several plausible-looking concerns by running them: every |
|
One more
|
Fixes for PR Revision:
Pre-existing / not blocking:
Cleanup:
|
|
Code review findings (recall-biased, high effort): 1.
|
|
Re-reviewed after Almost everything from the prior review is genuinely fixed — verified, not assumed:
One remaining
|
|
One more low-severity note on the new fp16/bf16 tree arms swallow decode errors to In practice this is latent: for a well-formed tensor, Minor test cleanup ( That's everything from my side — the |
…cleanup, delete unused encoders
Fixes:
Cleanup:
|
|
Two remaining findings from code review:
|
|
liftFloatUnop was missing a case for bfloat16, so trig/transcendental ops would throw on bf16. Added | .float16 | .bfloat16 => using the shared decode/encode helpers.