Skip to content
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

bpo-39288: Add math.nextafter(x, y) #17937

Merged
merged 1 commit into from Jan 12, 2020
Merged

bpo-39288: Add math.nextafter(x, y) #17937

merged 1 commit into from Jan 12, 2020

Conversation

@vstinner
Copy link
Member

vstinner commented Jan 10, 2020

Return the next floating-point value after x towards y.

https://bugs.python.org/issue39288

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 10, 2020

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 @requires_IEEE_754?

@mdickinson

This comment has been minimized.

Copy link
Member

mdickinson commented Jan 10, 2020

Maybe the test should be decorated ith @requires_IEEE_754?

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.

Copy link
@mdickinson

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.

@mdickinson

This comment has been minimized.

Copy link
Member

mdickinson commented Jan 10, 2020

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.

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

Please could we also have tests covering all the various corner cases: signed zeros, infinities, nans, subnormals, etc.?

I added more tests. Which tests do you want to signed zeros? The sign doesn't seem to matter:

>>> math.nextafter(0.0, 1.0)
5e-324
>>> math.nextafter(-0.0, 1.0)
5e-324
>>> math.nextafter(-0.0, -1.0)
-5e-324
>>> math.nextafter(+0.0, -1.0)
-5e-324

For subnormals, I wrote the following tests, is it what you expect?

        # around 0.0
        self.assertEqual(math.nextafter(0.0, INF),
                         sys.float_info.min * sys.float_info.epsilon)
        self.assertEqual(math.nextafter(0.0, -INF),
                         (-sys.float_info.min) * sys.float_info.epsilon)

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.)

Do you that if nextafter() behaves differently on a platform, Python should patch the function?

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 11, 2020

🤖 New build scheduled with the buildbot fleet by @vstinner for commit c12293b 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

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.

Let me try this new toy :-)

math.nextafter
x: float
y: float

This comment has been minimized.

Copy link
@vstinner

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.

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

I don't understand this result, but nextafter() gives me the expected result, whereas 1.0-epsilon gives me a surprising smaller value.

>>> math.nextafter(1.0, 2.0) == 1.0 + sys.float_info.epsilon
True
>>> math.nextafter(1.0, 0.0) == 1.0 - sys.float_info.epsilon
False
>>> math.nextafter(1.0, 0.0), 1.0 - sys.float_info.epsilon
(0.9999999999999999, 0.9999999999999998)
>>> math.nextafter(1.0, 0.0).hex(), (1.0 - sys.float_info.epsilon).hex()
('0x1.fffffffffffffp-1', '0x1.ffffffffffffep-1')
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

Good: the test passed on all buildbots!

buildbot/PPC64 Fedora PR failed on test_distutils, but it's unrelated to this PR:

@vstinner vstinner force-pushed the vstinner:nextafter branch from c12293b to 6bbcab5 Jan 11, 2020
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

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?

@vstinner vstinner force-pushed the vstinner:nextafter branch from 6bbcab5 to 1e78d64 Jan 11, 2020
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

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.

@mdickinson

This comment has been minimized.

Copy link
Member

mdickinson commented Jan 11, 2020

whereas 1.0-epsilon gives me a surprising smaller value.

Makes sense: assuming IEEE 754, the next Python float down from 1.0 is 1.0 - 0.5*epsilon, so 1.0 - epsilon is two floats away from 1.0.

Copy link
Member

mdickinson left a comment

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.

Copy link
@mdickinson

mdickinson Jan 11, 2020

Member

Suggest "is equal to" in place of "equals to"; it reads a bit better.

This comment has been minimized.

Copy link
@vstinner

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.
@vstinner vstinner force-pushed the vstinner:nextafter branch from 5944b91 to 2855a0f Jan 11, 2020
@mdickinson

This comment has been minimized.

Copy link
Member

mdickinson commented Jan 11, 2020

Do you that if nextafter() behaves differently on a platform, Python should patch the function?

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.

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

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.

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 11, 2020

@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.

@mdickinson

This comment has been minimized.

Copy link
Member

mdickinson commented Jan 11, 2020

I added tons of unit tests, so different behaviors will be easily catched.

Yep, the tests look very good. :-)

@vstinner vstinner merged commit 100fafc into python:master Jan 12, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200111.31 succeeded
Details
bedevere/issue-number Issue number 39288 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vstinner vstinner deleted the vstinner:nextafter branch Jan 12, 2020
@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 12, 2020

Thanks for the review @mdickinson and for helping to write unit tests.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 12, 2020

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x Debian 3.x has failed when building commit 100fafc.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/105/builds/167) and take a look at the build logs.
  4. Check if the failure is related to this commit (100fafc) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/105/builds/167

Failed tests:

  • test_asyncio

Failed subtests:

  • test_start_new_session - test.test_asyncio.test_subprocess.SubprocessMultiLoopWatcherTests

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/test/test_asyncio/test_subprocess.py", line 168, in test_start_new_session
    self.assertEqual(exitcode, 8)
AssertionError: 255 != 8


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-debian-z/build/Lib/asyncio/unix_events.py", line 1302, in _do_waitpid
    loop, callback, args = self._callbacks.pop(pid)
KeyError: 23823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.