Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39288: Add math.nextafter(x, y) #17937
Conversation
This comment has been minimized.
This comment has been minimized.
I'm not sure about the unit tests: not sure if they are portable on any IEEE 754 implementation, and if the test would work on non-IEEE 754 implementation. Maybe the test should be decorated ith |
This comment has been minimized.
This comment has been minimized.
Yes, I think so. There's nothing to guarantee that the existing test would work on a non IEEE 754 system, should Python ever encounter one. Please could we also have tests covering all the various corner cases: signed zeros, infinities, nans, subnormals, etc.? There's a lot that can go wrong, and I don't think it's safe to trust that the platform's libm does the right thing. (We've tried that before.) |
@@ -2033,6 +2033,12 @@ def testComb(self): | |||
self.assertIs(type(comb(IntSubclass(5), IntSubclass(k))), int) | |||
self.assertIs(type(comb(MyIndexable(5), MyIndexable(k))), int) | |||
|
|||
def test_nextafter(self): | |||
self.assertEqual(math.nextafter(9223372036854775807.0, 0.0), |
This comment has been minimized.
This comment has been minimized.
mdickinson
Jan 10, 2020
Member
It would be clearer to use a value that's exactly representable here: 9223372036854775808.0
rather than 9223372036854775807.0
. Otherwise there are two effects going on: first, the literal is being rounded to a different value, and then that value is having nextafter
applied to it.
This comment has been minimized.
This comment has been minimized.
Oh, and once we have more comprehensive tests, it might be worth trying out the new "test on the buildbots" functionality with this PR, to ferret out any platforms that have dodgy implementations of nextafter. |
This comment has been minimized.
This comment has been minimized.
I added more tests. Which tests do you want to signed zeros? The sign doesn't seem to matter:
For subnormals, I wrote the following tests, is it what you expect?
Do you that if nextafter() behaves differently on a platform, Python should patch the function? |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Jan 11, 2020
This comment has been minimized.
This comment has been minimized.
Let me try this new toy :-) |
math.nextafter | ||
x: float | ||
y: float |
This comment has been minimized.
This comment has been minimized.
vstinner
Jan 11, 2020
Author
Member
Oh. I didn't notice, that's a C float, whereas I mean a "Python float". In fact, I should use "double" here.
I will fix it once the buildbot tests will complete.
This comment has been minimized.
This comment has been minimized.
I don't understand this result, but nextafter() gives me the expected result, whereas 1.0-epsilon gives me a surprising smaller value.
|
This comment has been minimized.
This comment has been minimized.
Good: the test passed on all buildbots! buildbot/PPC64 Fedora PR failed on test_distutils, but it's unrelated to this PR: |
This comment has been minimized.
This comment has been minimized.
I fixed the implementation to use C double rather than C float, rebased my PR and squashed commits. @mdickinson: Would you mind to review the PR again? |
This comment has been minimized.
This comment has been minimized.
I'm not sure about "If x equals to y, return y": an implementation might return x. But I wrote an unit test to ensure that y is returned: the test using signed zeros. |
This comment has been minimized.
This comment has been minimized.
Makes sense: assuming IEEE 754, the next Python |
LGTM; one documentation nitpick. There's a suggestion on the issue to make the second argument optional. I'd suggest merging this PR as-is, and doing that in a separate PR if we decide that's the way we want to go. |
|
||
Return the next floating-point value after *x* towards *y*. | ||
|
||
If *x* equals to *y*, return *y*. |
This comment has been minimized.
This comment has been minimized.
mdickinson
Jan 11, 2020
Member
Suggest "is equal to" in place of "equals to"; it reads a bit better.
This comment has been minimized.
This comment has been minimized.
vstinner
Jan 11, 2020
Author
Member
Done. You may suggest this change to the Linux manual page of nextafter() as well where I stole this sentence :-)
Return the next floating-point value after x towards y.
This comment has been minimized.
This comment has been minimized.
My hope is that all platforms we care about follow the specification, but I'd at least like to know about it if there are platforms that don't. If it does turn out that some platforms deviate from the specification, we can make an informed decision about how to deal with them. But let's save that for when it happens. |
This comment has been minimized.
This comment has been minimized.
PR rebased to fix a conflict in Doc/whatsnew/3.9.rst and commits squashed. Once regular CI tests will pass, I will give a second try to buildbots since I added many tests since the previous run. |
This comment has been minimized.
This comment has been minimized.
@mdickinson: "If it does turn out that some platforms deviate from the specification, we can make an informed decision about how to deal with them. But let's save that for when it happens." Sure, I totally agree. I added tons of unit tests, so different behaviors will be easily catched. |
This comment has been minimized.
This comment has been minimized.
Yep, the tests look very good. :-) |
100fafc
into
python:master
This comment has been minimized.
This comment has been minimized.
Thanks for the review @mdickinson and for helping to write unit tests. |
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Jan 12, 2020
|
vstinner commentedJan 10, 2020
•
edited by bedevere-bot
Return the next floating-point value after x towards y.
https://bugs.python.org/issue39288