-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
gh-94906: Support multiple steps in math.nextafter #103881
Conversation
a96ad31
to
77fb4ae
Compare
It would be nice if steps could be negative rather than having to switch the sign of the second argument:
|
@mdickinson Please make the final decision on this. I still like the idea and would use it, but if you want to reject it, you have my support. |
@rhettinger I'd be happy to make negative steps work. I left it out for simplicity, because there are questions about semantics that would need to be resolved: The most obvious semantics would be for negative steps to move away from the target. Or are you suggesting that negative steps should be equivalent to negating the target argument? So for example:
Should infinity be absorbing, or should going beyond infinity give NaN? The former seems reasonable to me. But I can also come up with arguments for the latter. Similarly, we have That's rather different from We could throw a value error for all the weird cases, and allow negative n for the obvious cases only. But that seems like it might be hard to understand for users? |
Thanks @rhettinger. I'm short of time right now, but I'll take a fresh look at this this weekend. |
Okay, let's go ahead with this. Comments: I'm not totally convinced that But It's only recently that we've started assuming that the floating-point format in use by CPython is IEEE 754 binary64, which is what makes the type-punning via a union work. (Note that we're also assuming in this PR that uint64_t and double have matching endianness; in theory that's not guaranteed, but in practice it seems to be a fairly safe assumption. It may be worth a comment in the implementation, though.) On negative steps:
Agreed, and that's what I'd favour, but I also agree that the semantics aren't obvious. The more I think about it, the more this feels like an argument for having
Definitely absorbing. How about we get this in before the beta-1 cutoff and punt on allowing negative steps for now - we can always add those later. |
@mdickinson Seems all good to me.
We can do that in pure Python and stick it into a (non-standard) library, if there's demand for it? Even the multiple-steps-nextafter could live in a place that's not the standard library. I only took this PR to show off that you don't need a loop but can use some clever manipulation of the bits instead, not actually because I think this functionality needs to live in the standard library.
Works for me! Is there anything we need to change in this PR for that? |
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.
This generally looks good to me; a few comments.
The feature freeze is fast approaching. Is the ready to be applied? Once the core API is in, it is okay for the docs and tests continue to be tweaked. |
@rhettinger I implemented the review suggestions. |
There's a failed check "Tests / Ubunt SSL tests with OpenSSL (1.1.1t)" but it seems to be flaky and nothing to do with this PR. https://github.com/python/cpython/actions/runs/5001023587/jobs/8959184113?pr=103881 |
I noticed that we are supporting hypothesis property based testing now. I added some property tests. |
Hmm, I don't understand the test failures. They seem unrelated to what I did, but I am not sure. My tests work locally for me. |
Thanks for the updates; I'll re-review (and look into the bot failure) shortly. |
|
🤖 New build scheduled with the buildbot fleet by @mdickinson for commit 66ef3f9 🤖 If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
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.
Code LGTM; functionality works as expected in manual testing, and the bots seem happy. I'm not going to wait for the buildbots to finish, since that will take all night (literally).
* main: (30 commits) pythongh-103987: fix several crashes in mmap module (python#103990) docs: fix wrong indentation causing rendering error in dis page (python#104661) pythongh-94906: Support multiple steps in math.nextafter (python#103881) pythongh-104472: Skip `test_subprocess.ProcessTestCase.test_empty_env` if ASAN is enabled (python#104667) pythongh-103839: Allow building Tkinter against Tcl 8.7 without external libtommath (pythonGH-103842) pythongh-85984: New additions and improvements to the tty library. (python#101832) pythongh-104659: Consolidate python examples in enum documentation (python#104665) pythongh-92248: Deprecate `type`, `choices`, `metavar` parameters of `argparse.BooleanOptionalAction` (python#103678) pythongh-104645: fix error handling in marshal tests (python#104646) pythongh-104600: Make type.__type_params__ writable (python#104634) pythongh-104602: Add additional test for listcomp with lambda (python#104639) pythongh-104640: Disallow walrus in comprehension within type scopes (python#104641) pythongh-103921: Rename "type" header in argparse docs (python#104654) Improve readability of `typing._ProtocolMeta.__instancecheck__` (python#104649) pythongh-96522: Fix deadlock in pty.spawn (python#96639) pythonGH-102818: Do not call `PyTraceBack_Here` in sys.settrace trampoline. (pythonGH-104579) pythonGH-103545: Add macOS specific constants for ``os.setpriority`` to ``os`` (python#104606) pythongh-104623: Update macOS installer to SQLite 3.42.0 (pythonGH-104624) pythongh-104619: never leak comprehension locals to outer locals() (python#104637) pythongh-104602: ensure all cellvars are known up front (python#104603) ...
See issue for discussion.