Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 35.2k
gh-130317: fix PyFloat_Pack/Unpack[24] for NaN's with payload#130452
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
971fd98b05720af513b14218f6b3a7f5ed9c7c08ffde7b6aae6c1c12ac7bdcbb461b81cbfbf056a5dc1a86ab7c332bdeed773f51eb27c9164dc56976f71e343e5a211092d7296137c7500dfd66b7f727c13e8310bcf54f69d20633File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import math | ||
| import random | ||
| import sys | ||
| import unittest | ||
| import warnings | ||
| @@ -178,6 +179,39 @@ def test_pack_unpack_roundtrip(self): | ||
| else: | ||
| self.assertEqual(value2, value) | ||
| @unittest.skipUnless(HAVE_IEEE_754, "requires IEEE 754") | ||
| def test_pack_unpack_roundtrip_for_nans(self): | ||
| pack = _testcapi.float_pack | ||
| unpack = _testcapi.float_unpack | ||
| for _ in range(1000): | ||
| for size in (2, 4, 8): | ||
| sign = random.randint(0, 1) | ||
| signaling = random.randint(0, 1) | ||
| quiet = int(not signaling) | ||
| if size == 8: | ||
| payload = random.randint(signaling, 1 << 50) | ||
| i = (sign << 63) + (0x7ff << 52) + (quiet << 51) + payload | ||
| elif size == 4: | ||
| payload = random.randint(signaling, 1 << 21) | ||
| i = (sign << 31) + (0xff << 23) + (quiet << 22) + payload | ||
| elif size == 2: | ||
| payload = random.randint(signaling, 1 << 8) | ||
| i = (sign << 15) + (0x1f << 10) + (quiet << 9) + payload | ||
| data = bytes.fromhex(f'{i:x}') | ||
| for endian in (BIG_ENDIAN, LITTLE_ENDIAN): | ||
| with self.subTest(data=data, size=size, endian=endian): | ||
| data1 = data if endian == BIG_ENDIAN else data[::-1] | ||
| value = unpack(data1, endian) | ||
| if signaling and sys.platform == 'win32': | ||
skirpichev marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| # On this platform sNaN becomes qNaN when returned | ||
| # from function. That's a known bug, e.g. | ||
| # https://developercommunity.visualstudio.com/t/155064 | ||
skirpichev marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. skirpichev marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| # (see also gh-130317). | ||
| value = _testcapi.float_set_snan(value) | ||
| data2 = pack(size, value, endian) | ||
| self.assertTrue(math.isnan(value)) | ||
| self.assertEqual(data1, data2) | ||
| if __name__ == "__main__": | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| Fix :c:func:`PyFloat_Pack2` and :c:func:`PyFloat_Unpack2` for NaN's with | ||
| payload. This corrects round-trip for :func:`struct.unpack` and | ||
| :func:`struct.pack` in case of the IEEE 754 binary16 "half precision" type. | ||
| Patch by Sergey B Kirpichev. |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100 tests should be enough to validate the implementation, no?
1000 tests might be a little bit too slow, I don't think that it's worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you, the test looks instantaneous on my system. 0.3sec vs 0.03. Where the threshold?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think speed is really an issue here, but having the potential for 6000 failed test reports seems like major overkill. I think 10 would actually be plenty for this loop.