Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-94906: Support multiple steps in math.nextafter#103881

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
mdickinson merged 23 commits intopython:mainfrommatthiasgoergens:gh-94906
May 19, 2023

Conversation

@matthiasgoergens
Copy link
Contributor

@matthiasgoergensmatthiasgoergens commentedApr 26, 2023
edited by bedevere-bot
Loading

@rhettinger
Copy link
Contributor

It would be nice ifsteps could be negative rather than having to switch the sign of the second argument:

# Current waynearby = [nextafter(x, -inf, steps=2),          nextafter(x, -inf, steps=1),          x,          nextafter(x, inf, steps=1),          nextafter(x, inf, steps=2),]# Better waynearby = [nextafter(x, inf, steps=n) for n in (-2, -1, 0, 1, 2)]

@rhettinger
Copy link
Contributor

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

@matthiasgoergens
Copy link
ContributorAuthor

matthiasgoergens commentedApr 27, 2023
edited
Loading

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

nextafter(x, 0.0, -1) == nextafter(x, -0.0, 1)

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 havenextafter(x, x, n) == x for all x and all n. (Ignoring that NaNs don't compare with == equality.) Including x=inf.

That's rather different fromnextafter(x, -x, -n) for most x and most n.

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?

@mdickinson
Copy link
Member

Thanks@rhettinger. I'm short of time right now, but I'll take a fresh look at this this weekend.

rhettinger reacted with thumbs up emoji

@mdickinson
Copy link
Member

Okay, let's go ahead with this.

Comments: I'm not totally convinced thatnextafter is the right place for this functionality - I suspect the real need is to either add or subtract "n" ulps from a given value, whilenextafter is doing something more complicated than that (e.g., saturating when it hits the target value). I'd be open to suggestions for alternative spellings for the functionality. (Aside: an obvious extension of this functionality would be the ability to subtract one float from another and get a result expressed in an integer number of ulps.)

Butnextafter itself is already a slightly odd beast, in that I don't think the generality of the second argument is valuable - I suspect there are few use-cases that aren't one of next-towards-plus-infinity / next-towards-negative-infinity / next-towards-zero / next-away-from-zero (and that last one isn't even expressible cleanly withnextafter). Maybe that's why IEEE 754 went with nextUp and nextDown instead of following C's nextafter.

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

@matthiasgoergens

On negative steps:

The most obvious semantics would be for negative steps to move away from the target.

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 havingnextdown andnextup instead ofnextafter (or even justnextup), where itis obvious what negative steps would mean.

Should infinity be absorbing, or should going beyond infinity give NaN?

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.

@matthiasgoergens
Copy link
ContributorAuthor

@mdickinson Seems all good to me.

(Aside: an obvious extension of this functionality would be the ability to subtract one float from another and get a result expressed in an integer number of ulps.)

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.

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.

Works for me! Is there anything we need to change in this PR for that?

@mdickinsonmdickinson self-requested a reviewMay 7, 2023 09:56
Copy link
Member

@mdickinsonmdickinson left a 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.

@rhettinger
Copy link
Contributor

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.

@matthiasgoergens
Copy link
ContributorAuthor

@rhettinger I implemented the review suggestions.

@matthiasgoergens
Copy link
ContributorAuthor

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

mdickinson reacted with thumbs up emoji

@matthiasgoergens
Copy link
ContributorAuthor

I noticed that we are supporting hypothesis property based testing now. I added some property tests.

mdickinson reacted with thumbs up emoji

@matthiasgoergens
Copy link
ContributorAuthor

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.

@mdickinson
Copy link
Member

Thanks for the updates; I'll re-review (and look into the bot failure) shortly.

@mdickinson
Copy link
Member

mdickinson commentedMay 19, 2023
edited
Loading

@matthiasgoergens

Hmm, I don't understand the test failures. [...]

make patchcheck was failing on a whitespace error (a stray TAB character); I've fixed it in6527b77.

matthiasgoergens reacted with heart emoji

@mdickinsonmdickinson added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 19, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@mdickinson for commit66ef3f9 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelMay 19, 2023
Copy link
Member

@mdickinsonmdickinson left a 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).

@mdickinsonmdickinson merged commit6e39fa1 intopython:mainMay 19, 2023
carljm added a commit to gsallam/cpython_with_perfmap_apii that referenced this pull requestMay 20, 2023
* 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)  ...
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@mdickinsonmdickinsonmdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mdickinsonmdickinson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@matthiasgoergens@rhettinger@mdickinson@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp