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

RFC: add nextafter #664

Closed
steff456 opened this issue Jul 26, 2023 · 5 comments · Fixed by #792
Closed

RFC: add nextafter #664

steff456 opened this issue Jul 26, 2023 · 5 comments · Fixed by #792
Labels
API extension Adds new functions or objects to the API. Needs Discussion Needs further discussion. RFC Request for comments. Feature requests and proposed changes.
Milestone

Comments

@steff456
Copy link
Member

steff456 commented Jul 26, 2023

This RFC proposes adding nextafter function

Overview

Based on array comparison data, the API is available in most array libraries. The main exception is MXNet which doesn't implement it.

Prior art

  • MXNet doesn't have an implementation for nextafter

Proposal

This proposal follows similar element-wise APIs,

def nextafter(x1: array, x2: array, /) -> array

Related

@steff456 steff456 added the API extension Adds new functions or objects to the API. label Jul 26, 2023
@kgryte
Copy link
Contributor

kgryte commented Jul 27, 2023

A few questions come to mind:

@rgommers
Copy link
Member

rgommers commented Jul 27, 2023

My thoughts:

  • The Julia API is nicest and usually what you want. However, I don't really have much of a desire to see a new function here that doesn't have precedent in all existing Python libraries. It's a pretty simple API with no keywords, so it doesn't really matter that it's not completely optimal.
  • The proposal say array not array | scalar. This function is used with scalars a lot. array is consistent with our whole API design though, and PyTorch does not support scalars. On the other hand, this is going to cause a lot of churn for array-consuming code, where for example np.nextafter(x, -np.inf) has to become np.nextafter(x, np.array(-np.inf, dtype=x.dtype)) or some such thing. This is the trickiest thing to decide. I think we may consider allowing x2 to be a Python scalar.
  • Type promotion: normal type promotion rules between x1 and x2 for floating-point dtypes - that's what I expect, and I just checked that NumPy, JAX and PyTorch all support it.
  • Integer x1 dype: it's ambiguous and at least PyTorch does not support it, so undefined behavior (may raise an exception).
  • Integer x2 dtype should be allowed, since it's just indicating the direction.

@kgryte
Copy link
Contributor

kgryte commented Jul 27, 2023

Type promotion: normal type promotion rules between x1 and x2 for floating-point dtypes - that's what I expect, and I just checked that NumPy, JAX and PyTorch all support it.

To my eyes, that is weird behavior. I would think that what you usually want is the next value before/after an element in x1 with the same dtype. To provide a float32 array for x1 and, e.g., a float64 array for x2 and get back a float64 array strikes me as not what is usually wanted, as the entire point of nextafter is to get the nearest floating-point value to an input. If you want the next float64 value, IMO, you should provide a float64 input array for x1.

@kgryte
Copy link
Contributor

kgryte commented Jul 27, 2023

Integer x2 dtype should be allowed, since it's just indicating the direction.

This seems in conflict with supporting type promotion, as mixed-kind promotion semantics are undefined.

@rgommers
Copy link
Member

To my eyes, that is weird behavior. I would think that what you usually want is the next value before/after an element in x1 with the same dtype.

On second thought, I agree with you there.

@kgryte kgryte added this to the v2024 milestone Mar 21, 2024
@kgryte kgryte added Needs Discussion Needs further discussion. RFC Request for comments. Feature requests and proposed changes. labels Apr 4, 2024
kgryte added a commit that referenced this issue May 2, 2024
PR-URL: #792
Closes: #664
Reviewed-by: Ralf Gommers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API extension Adds new functions or objects to the API. Needs Discussion Needs further discussion. RFC Request for comments. Feature requests and proposed changes.
Projects
Status: Stage 1
Development

Successfully merging a pull request may close this issue.

3 participants