FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-39288: Add math.nextafter(x, y) by vstinner · Pull Request #17937 · python/cpython · GitHub

Repository navigation

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

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:nextafter
Jan 12, 2020
Merged

vstinner merged 1 commit into
python:masterfrom
vstinner:nextafter

Conversation

vstinner commented Jan 10, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

https://bugs.python.org/issue39288

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member

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

Comment thread Lib/test/test_math.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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?

vstinner added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 11, 2020

Copy link
Copy Markdown

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

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

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jan 11, 2020

Copy link
Copy Markdown
Member Author

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

Comment thread Modules/mathmodule.c Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

Good: the test passed on all buildbots!

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

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member

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.

mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Doc/library/math.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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.

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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

Yep, the tests look very good. :-)

vstinner merged commit 100fafc into python:master Jan 12, 2020
vstinner deleted the nextafter branch January 12, 2020 01:15

Copy link
Copy Markdown
Member Author

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

python deleted a comment from bedevere-bot Jan 12, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
Return the next floating-point value after x towards y.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL