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

Consider removing array constants #154

Closed
shoyer opened this issue Mar 27, 2021 · 10 comments
Closed

Consider removing array constants #154

shoyer opened this issue Mar 27, 2021 · 10 comments
Labels
API change Changes to existing functions or objects in the API.

Comments

@shoyer
Copy link
Contributor

shoyer commented Mar 27, 2021

I stumbled across this page, which lists four constants in the spec: e, inf, nan and pi.

I'm not strongly opposed, but it isn't really clear to me why we need these in the spec when they already exist in Python's builtin math built. Given that these should be builtin Python float objects, it isn't clear to me what we gain from including these, except saving users another import?

@rgommers
Copy link
Member

Mostly convenience indeed. Every library has them, users expect them, and they're used a lot. Writing

y = xp.sin(x + xp.pi/4)

seems more natural than y = xp.sin(x + math.pi/4).

These would otherwise the only things we'd rely on that are not Python builtins I believe.

@rgommers rgommers added the API change Changes to existing functions or objects in the API. label Mar 29, 2021
@shoyer
Copy link
Contributor Author

shoyer commented Mar 29, 2021

I'm not sure what you mean by "every library has them." As far as I can tell, only libraries that directly try to copy NumPy's API do. E.g., TensorFlow does not.

@rgommers
Copy link
Member

Argh, you're right - PyTorch also doesn't. Although I'm not sure why not, the test suite is full of nan = float('nan') which is pretty clumsy.

@rgommers
Copy link
Member

I could live with leaving these out, but personally lean towards leaving them in for convenience. And because I don't like the math module much, would rather not rely on anything in the stdlib.

@rgommers
Copy link
Member

Would be nice to hear what maintainers of other libraries think. From a NumPy and its downstream libraries perspective these are expected, but if there's strong feelings especially from the libraries that don't have them then it could go the other way. I checked with PyTorch, they'd be fine with an alias. @edloper for TensorFlow, any problem with adding these constants?

@edloper
Copy link

edloper commented Apr 29, 2021

I don't have a strong opinion on whether these constants be included in the spec or not.

If they are included in the spec, then are they required to be python float values? Or array values? Or is it up to the library? If array values, then are there any requirements on what dtype they have? Note that many libraries don't do automatic casting between dtypes, so if a library defines pi to be a float32 scalar, then users can't combine it with a float64 value (without casting), and vice versa.

@shoyer
Copy link
Contributor Author

shoyer commented Apr 29, 2021

I think they should be required to be Python float values (which effectively means float64).

@asmeurer
Copy link
Member

Note that many libraries don't do automatic casting between dtypes, so if a library defines pi to be a float32 scalar, then users can't combine it with a float64 value (without casting), and vice versa.

The spec requires type promotion. https://data-apis.org/array-api/latest/API_specification/type_promotion.html.

@rgommers
Copy link
Member

For clarification:

  • float_scalar + array_float32 -> float32
  • float_scalar + array_float64 -> float64
  • int_scalar + array_float32 -> float32
  • int_scalar + array_float64 -> float64

Python scalars always have the lowest precedence, so they should not change array dtype (unless you do something like float_scalar + array_intxx, that should result in an array with the default floating-point dtype.

@rgommers
Copy link
Member

rgommers commented May 6, 2021

We discussed this in a call today. @kkraus14 had a strong preference for keeping these constants in, because he wants to be able to implement them as a custom Python object that's a singleton (and duck-types as a float, e.g. by implementing __float__). That way, once it's on a GPU it can be reused there and this saves implicit syncs, compared to using math.pi et al.

Since no one is strongly opposed to keeping these constants in the API, that's a good reason to stay with the status quo and keep them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change Changes to existing functions or objects in the API.
Projects
None yet
Development

No branches or pull requests

4 participants