gh-154568: Fix array._array_reconstructor() ignoring requested byte order for float16 - #154569
Conversation
…byte order for float16
The slow-path decoder for IEEE_754_FLOAT16_LE/BE computed the
byte-order flag by comparing mformat_code against IEEE_754_FLOAT_LE
(the 32-bit float constant) instead of IEEE_754_FLOAT16_LE. Since the
float16 mformat codes are never equal to that constant, the comparison
was always false, so the decoder always treated input as big-endian
regardless of what was requested.
Adds a regression test. A real 'e'-typecode array only exercises the
slow/converting path when the requested mformat disagrees with the
machine's native float16 format, so which direction (LE/BE) exercises
the buggy branch depends on the test machine's endianness, and the
other direction happens to come out "correct" by coincidence since the
bug unconditionally decodes as big-endian. The test uses a typecode
('d') whose native format can never match the float16 codes, forcing
the slow path deterministically on any machine.
This comment was marked as resolved.
This comment was marked as resolved.
…ction array._array_reconstructor is internal/private and has no Sphinx docs entry, so the :func: cross-reference role can't resolve and trips the docs CI's new-NEWS-nit check.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
skirpichev
added
the
needs backport to 3.15
|
CC @vstinner |
…lify NEWS wording Per skirpichev's review on pythonGH-154569: - Add a float16 (LE/BE) case to ArrayReconstructorTest.test_numbers() for basic functional coverage alongside the other numeric types, complementing the existing dedicated test_float16_endianness (which he confirmed is fine to keep for deterministically forcing the buggy slow path regardless of test-machine endianness). - Simplify the NEWS entry wording per his suggested rewrite.
|
Thanks for the review! Pushed both changes: added a half-floats case to |
|
Good catch, fixed the gh-issue placeholder in the test comment. That |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
skirpichev
left a comment
skirpichev
left a comment
There was a problem hiding this comment.
Spammer reported. Meanwhile, I locked pr to collaborators, sorry.
Sadly, I cannot comment the PR because of that. So I unlocked the conversation. |
|
Thanks — shortened the NEWS entry and test comment, and switched the test value to 1.5 as suggested. |
|
Thanks @PhysicistJohn for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.15. |
|
GH-155557 is a backport of this pull request to the 3.15 branch. |
bedevere-app
Bot
removed
the
needs backport to 3.15
|
Merged, thanks for the fix. |
4 participants
The slow-path decoder for IEEE_754_FLOAT16_LE/BE compared mformat_code
against IEEE_754_FLOAT_LE (the 32-bit float constant) instead of
IEEE_754_FLOAT16_LE, so it always decoded as big-endian regardless of
what was requested. Fixes gh-154568.
Adds a regression test. A real 'e'-typecode array only exercises the
slow/converting path when the requested mformat disagrees with the
machine's native float16 format, so which direction (LE/BE) actually
exercises the buggy branch depends on the test machine's endianness --
and the other direction happens to come out "correct" by coincidence,
since the bug unconditionally decodes as big-endian. The test uses a
typecode ('d') whose native format can never match the float16 codes,
which forces the slow path deterministically on any machine so both
directions are actually exercised.