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

Let math.nextafter() compute multiple steps at a time. #94906

Closed
rhettinger opened this issue Jul 16, 2022 · 18 comments
Closed

Let math.nextafter() compute multiple steps at a time. #94906

rhettinger opened this issue Jul 16, 2022 · 18 comments
Labels
3.12 bugs and security fixes type-feature A feature request or enhancement

Comments

@rhettinger
Copy link
Contributor

rhettinger commented Jul 16, 2022

Sometimes math.nextafter() needs to be applied multiple times in succession.

   x = nextafter(nextafter( nextafter(x, inf), inf), inf)    # Three steps up

It would be nice if the function supported this directly:

   x = nextafter(x, inf, n=3)

The implementation would just be a for-loop:

def newnextafter(x, y, /, *, n=1):
    'Return the floating-point value n steps after x towards y.'
    for i in range(n):
        x = nextafter(x, y)
    return x

The formal paramater can be just n or the longer but more descriptive steps.

Linked PRs

@rhettinger rhettinger added type-feature A feature request or enhancement easy 3.12 bugs and security fixes labels Jul 16, 2022
hauntsaninja added a commit to hauntsaninja/cpython that referenced this issue Jul 16, 2022
@mdickinson
Copy link
Member

mdickinson commented Jul 17, 2022

This feels like a needless expansion of the API to me; I don't think I've ever encountered a use-case for this. On the rare occasions that such a use-case turns up, what's wrong with the for loop, or with adding an integer multiple of math.ulp(x) to x?

-1 from me.

@rhettinger
Copy link
Contributor Author

I wouldn't posted the issue if I hadn't needed this, so it is not "needless".

In 30 seconds of searching, I found another example:

https://github.com/ericherman/libefloat/blob/ad606d566ac5310aabb4fff8890e35200a718e8d/tests/test-distance-64.c#L94

Python is a high level language and it is suitable to incorporate options that are more expansive than used in low level languages like C that tend to focus on atomic steps.

@mdickinson
Copy link
Member

Sure, I don't doubt that you had a genuine need, but that need is trivially addressed with a for loop. My use of "needless" referred to the expansion of the API. You needing the functionality is not the same thing as the math module needing to support the functionality directly.

Sorry, but I don't see this use-case as valuable enough or common enough to warrant expanding the math.nextafter API.

I'm also concerned that this might be a performance trap: a user could easily write nextafter(x, y, steps=10**6) without realising that that means a million invocations of nextafter internally. With an explicit for loop, the effect on running time is much more obvious.

@serhiy-storchaka
Copy link
Member

I concur with @mdickinson. It is too niche feature, and it is not difficult to implement a wrapper in Python. BTW, the case you found is in tests.

@rhettinger
Copy link
Contributor Author

I wish this wasn't dismissed so casually. All uses of nextafter() are "niche". There are only a handful of people who will ever use it.

I submitted the feature request because I needed to write the replacement function in pure Python (much like the example I posted above) and it was distracting and felt amiss, like something the should have already have been built in to a higher level language.

I wrote the function while working on another problem and had casually created an incorrect result along the way:

>>> sixth_largest_random = 1.0 - 6 * ulp(1.0)
>>> sixth_largest_random
0.9999999999999987

Fortunately, I spotted the problem before going too far with it and wrote the above function giving the correct answer:

>>> sixth_largest_random = newnextafter(1.0, -inf, n=6)
>>> sixth_largest_random
0.9999999999999993

The entire side trip was distracting and annoying. It took the focus away for the error analysis I was working on at the time.

The proposal is an easy thing to do. It isn't even slightly confusing. It would be handy when needed by the few who ever use this function. I don't see any downside.

Note, in numpy and scipy, most interesting functions have many options. That seems to work well for them. But in Python, there seems to be an urge to fight against the simplest of options as "unnecessary API expansion".

@matthiasgoergens
Copy link
Contributor

matthiasgoergens commented Aug 15, 2022

@rhettinger @hauntsaninja @mdickinson

You don't need a loop for this. See this pure Python prototype inspired by an old StackOverflow question of mine.

And here's a prototype of a fix of the PR. (Please keep in mind that the C code in the PR isn't tested nor even run. The pure Python version has Hypothesis tests, so it's much more reliable.)

matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue Aug 15, 2022
matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue Aug 15, 2022
@ambv ambv removed the easy label Apr 24, 2023
@ambv
Copy link
Contributor

ambv commented Apr 24, 2023

Removing "easy" for now as this needs a resolution between the two maintainers of the math module.

@mblahay
Copy link
Contributor

mblahay commented Apr 24, 2023

@rhettinger @mdickinson

If the goal here is to produce a one line implementation capable of looping the execution of math.next after, it can be achieved using the reduce function from func tools.

from math import nextafter,inf
from functools import reduce

reduce(nextafter,[-inf]*6,1.0)

The starting point, 1.0, is the third argument, then you must provide an iterator that outputs the direction of nextafter for the desired number of times, in this case I use a list and duplicate -inf 6 times.

@matthiasgoergens
Copy link
Contributor

Reduce still loops internally.

@pochmann
Copy link
Contributor

pochmann commented Apr 25, 2023

See this pure Python prototype

Bugfix for the tests, applied by now

The tests are broken. You assert the following ("soll"/"ist" are expected and actual result (better use English)):

assert math.isnan(soll) == math.isnan(ist) or soll == ist

If both values aren't nan, then math.isnan(soll) == math.isnan(ist) is true, and soll == ist doesn't get checked! In fact, I modified your solution code to end with the totally wrong

    result = (the expression you return)
    return result if math.isnan(result) else 42

and the tests didn't catch it.

You should change == to and:

assert math.isnan(soll) and math.isnan(ist) or soll == ist

Btw, reduce+repeat is about 2-3 times faster than the loop, making your tests run faster (and avoiding the DeadlineExceeded error I get with the loop):

from functools import reduce
from itertools import repeat

def nextafter_reduce(start, goal, steps):
    return reduce(math.nextafter, repeat(goal, steps), start)

@matthiasgoergens
Copy link
Contributor

Thanks for noticing the bug!

@mblahay
Copy link
Contributor

mblahay commented Apr 25, 2023

I want to make sure we stay focused here. Though the original reporter is trying to avoid writing the loop, and others indicate the required loop is small enough that it is of no consequence; the Pure Python Prototype is being pushed as a solution despite being an entire new module. This would seem to be the wrong direction.

What happens here? The issue is coming up on its first birthday. The question for users boils down to this: to loop or not to loop. What is the mechanism that is used to decide when there is this kind of division?

@matthiasgoergens
Copy link
Contributor

matthiasgoergens commented Apr 25, 2023

The pure Python prototype would turn into a small C function. Not an entire new module.

matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue Apr 25, 2023
@matthiasgoergens
Copy link
Contributor

matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue Apr 25, 2023
@rhettinger
Copy link
Contributor Author

Removing "easy" for now as this needs a resolution between the two maintainers of the math module.

FWIW, I only care about this a tiny bit. Though I think it would be nice to have, I won't lose any sleep if this gets closed.

@matthiasgoergens
Copy link
Contributor

matthiasgoergens commented Apr 25, 2023

I overhauled the C PR https://github.com/matthiasgoergens/cpython/pull/5/files Tests pass now.

I'll point it at the upstream CPython repository, after I polish the git history.

matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue May 2, 2023
matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue May 17, 2023
matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue May 17, 2023
matthiasgoergens added a commit to matthiasgoergens/cpython that referenced this issue May 18, 2023
mdickinson added a commit that referenced this issue May 19, 2023
This PR updates `math.nextafter` to add a new `steps` argument. The behaviour is as though `math.nextafter` had been called `steps` times in succession.

---------

Co-authored-by: Mark Dickinson <mdickinson@enthought.com>
carljm added a commit to gsallam/cpython_with_perfmap_apii that referenced this issue 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)
  ...
@mdickinson
Copy link
Member

Done in #103881. Thanks to @matthiasgoergens.

@matthiasgoergens
Copy link
Contributor

Done in #103881. Thanks to @matthiasgoergens.

Thanks for fixing the PR up at the end, too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

7 participants