Skip to content

gh-111495: Add tests for PyFloat C API #111624

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Nov 5, 2023
Merged

gh-111495: Add tests for PyFloat C API #111624

merged 13 commits into from
Nov 5, 2023

Conversation

skirpichev
Copy link
Contributor

@skirpichev skirpichev commented Nov 2, 2023 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Nov 2, 2023
Closed
10 tasks
@skirpichev
Copy link
Contributor Author

Some C API tests for floats are in Lib/test/test_float.py historically. An example:

cpython/Lib/test/test_float.py

Lines 1518 to 1569 in ed587be

@unittest.skipIf(_testcapi is None, 'needs _testcapi')
class PackTests(unittest.TestCase):
def test_pack(self):
self.assertEqual(_testcapi.float_pack(2, 1.5, BIG_ENDIAN),
b'>\x00')
self.assertEqual(_testcapi.float_pack(4, 1.5, BIG_ENDIAN),
b'?\xc0\x00\x00')
self.assertEqual(_testcapi.float_pack(8, 1.5, BIG_ENDIAN),
b'?\xf8\x00\x00\x00\x00\x00\x00')
self.assertEqual(_testcapi.float_pack(2, 1.5, LITTLE_ENDIAN),
b'\x00>')
self.assertEqual(_testcapi.float_pack(4, 1.5, LITTLE_ENDIAN),
b'\x00\x00\xc0?')
self.assertEqual(_testcapi.float_pack(8, 1.5, LITTLE_ENDIAN),
b'\x00\x00\x00\x00\x00\x00\xf8?')
def test_unpack(self):
self.assertEqual(_testcapi.float_unpack(b'>\x00', BIG_ENDIAN),
1.5)
self.assertEqual(_testcapi.float_unpack(b'?\xc0\x00\x00', BIG_ENDIAN),
1.5)
self.assertEqual(_testcapi.float_unpack(b'?\xf8\x00\x00\x00\x00\x00\x00', BIG_ENDIAN),
1.5)
self.assertEqual(_testcapi.float_unpack(b'\x00>', LITTLE_ENDIAN),
1.5)
self.assertEqual(_testcapi.float_unpack(b'\x00\x00\xc0?', LITTLE_ENDIAN),
1.5)
self.assertEqual(_testcapi.float_unpack(b'\x00\x00\x00\x00\x00\x00\xf8?', LITTLE_ENDIAN),
1.5)
def test_roundtrip(self):
large = 2.0 ** 100
values = [1.0, 1.5, large, 1.0/7, math.pi]
if HAVE_IEEE_754:
values.extend((INF, NAN))
for value in values:
for size in (2, 4, 8,):
if size == 2 and value == large:
# too large for 16-bit float
continue
rel_tol = EPSILON[size]
for endian in (BIG_ENDIAN, LITTLE_ENDIAN):
with self.subTest(value=value, size=size, endian=endian):
data = _testcapi.float_pack(size, value, endian)
value2 = _testcapi.float_unpack(data, endian)
if isnan(value):
self.assertTrue(isnan(value2), (value, value2))
elif size < 8:
self.assertTrue(math.isclose(value2, value, rel_tol=rel_tol),
(value, value2))
else:
self.assertEqual(value2, value)

In this PR this left intact, but maybe we should move them.

sobolevn
sobolevn reviewed Nov 2, 2023
View reviewed changes
@skirpichev skirpichev mentioned this pull request Nov 3, 2023
Closed
@skirpichev skirpichev requested a review from sobolevn November 3, 2023 02:52
sobolevn
sobolevn reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one suggestion :)
All other things LGTM.

skirpichev
skirpichev commented Nov 3, 2023
View reviewed changes
@skirpichev skirpichev mentioned this pull request Nov 4, 2023
Closed
@skirpichev skirpichev marked this pull request as draft November 4, 2023 12:26
@skirpichev
Copy link
Contributor Author

Converted to a draft per review #111591

serhiy-storchaka
serhiy-storchaka reviewed Nov 4, 2023
View reviewed changes
@skirpichev skirpichev marked this pull request as ready for review November 4, 2023 15:48
serhiy-storchaka
serhiy-storchaka reviewed Nov 4, 2023
View reviewed changes
serhiy-storchaka
serhiy-storchaka reviewed Nov 5, 2023
View reviewed changes
serhiy-storchaka
serhiy-storchaka approved these changes Nov 5, 2023
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Thank you @skirpichev for your contribution and for your fast response.

@serhiy-storchaka serhiy-storchaka added tests Tests in the Lib/test dir skip news needs backport to 3.12 only security fixes topic-C-API labels Nov 5, 2023
@serhiy-storchaka serhiy-storchaka merged commit b452202 into python:main Nov 5, 2023
@miss-islington-app
Copy link

Thanks @skirpichev for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 5, 2023
@skirpichev @miss-islington
pythongh-111495: Add tests for PyFloat C API (pythonGH-111624)
0bff358 
(cherry picked from commit b452202)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Nov 5, 2023

GH-111752 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 5, 2023
@skirpichev skirpichev deleted the capi-float-tests branch November 5, 2023 07:21
serhiy-storchaka pushed a commit that referenced this pull request Nov 5, 2023
@miss-islington @skirpichev
[3.12] gh-111495: Add tests for PyFloat C API (GH-111624) (GH-111752)
fef32a1 
(cherry picked from commit b452202)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@skirpichev skirpichev mentioned this pull request Nov 6, 2023
Closed
@skirpichev
Copy link
Contributor Author

@serhiy-storchaka, thank you for review.

@skirpichev skirpichev mentioned this pull request Nov 7, 2023
Merged
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@skirpichev @aisk
pythongh-111495: Add tests for PyFloat C API (pythonGH-111624)
6b82c0d
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@skirpichev @Glyphack
pythongh-111495: Add tests for PyFloat C API (pythonGH-111624)
7795860
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn left review comments

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir topic-C-API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skirpichev @serhiy-storchaka @sobolevn