-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug in pack_float16()
#157
Comments
Debugging here. Python's struct.(un)pack can handle the value correctly:
It is definitely an issue with the c implementation. Encoding with the pure python encoder (
So it's probably somewhere here: Line 1689 in cabfde5
|
Just commenting for future me (or anyone else interested in debugging): I started the debugger to figure out where its going wrong, and I'll try to find time to continue that. But for anyone that is interested, this is basically how to get started with debugging: https://llllllllll.github.io/c-extension-tutorial/gdb.html
Bringing it down to unpack_float16, which encodes
Check: https://evanw.github.io/float-toy/ (thanks @Sekenre ). Bit 11 is wrong in the unpack_float16 |
I did some cursory testing of your float16 functions (out of curiosity, since I recently wrote my own conversion functions float16 <-> float32).
It seems your
pack_float16()
doesn't give the same answer as mine for some values. For small numbers, it could maybe be attributed to different rounding rules, but for high numbers it does seem wrong, for example for the float32 number represented in raw hex as0x47000000
, i.e.32768.0
, which has an exact representation in float16:0x7800
, but is rounded to infinity bypack_float16()
.I did read the document you referenced in
half_float_tables.py
, and it does not specify any rounding mode, so I don't know how it handles that. My code is written to do "Round to nearest, ties to even", which I tested against numpy.I looked at the code and realized that
pack_float16()
being wrong is not critical, I just wanted to let you know.If you're curious, my code and tests can be found here: NordicSemiconductor/zcbor#285
The text was updated successfully, but these errors were encountered: