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

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

Merged
merged 23 commits into from
May 19, 2023

Conversation

matthiasgoergens
Copy link
Contributor

@matthiasgoergens matthiasgoergens commented Apr 26, 2023

@rhettinger
Copy link
Contributor

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

# Current way
nearby = [nextafter(x, -inf, steps=2),
          nextafter(x, -inf, steps=1),
          x,
          nextafter(x, inf, steps=1),
          nextafter(x, inf, steps=2),
]

# Better way
nearby = [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
Contributor Author

matthiasgoergens commented Apr 27, 2023

@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 have nextafter(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 from nextafter(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.

@mdickinson
Copy link
Member

Okay, let's go ahead with this.

Comments: I'm not totally convinced that nextafter is the right place for this functionality - I suspect the real need is to either add or subtract "n" ulps from a given value, while nextafter 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.)

But nextafter 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 with nextafter). 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 having nextdown and nextup instead of nextafter (or even just nextup), where it is 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
Contributor Author

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

@mdickinson mdickinson self-requested a review May 7, 2023 09:56
Copy link
Member

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

Modules/mathmodule.c Outdated Show resolved Hide resolved
Modules/mathmodule.c Outdated Show resolved Hide resolved
Modules/mathmodule.c Show resolved Hide resolved
Doc/library/math.rst Show resolved Hide resolved
@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
Contributor Author

@rhettinger I implemented the review suggestions.

@matthiasgoergens
Copy link
Contributor Author

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

@matthiasgoergens
Copy link
Contributor Author

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

@matthiasgoergens
Copy link
Contributor Author

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.

Modules/mathmodule.c Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member

mdickinson commented May 19, 2023

@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 in 6527b77.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 19, 2023
@bedevere-bot
Copy link

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

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

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

@mdickinson mdickinson merged commit 6e39fa1 into python:main May 19, 2023
20 checks passed
carljm added a commit to gsallam/cpython_with_perfmap_apii that referenced this pull request May 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 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.

None yet

4 participants