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

feat: add scalar support to element-wise functions #862

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Nov 25, 2024

This PR

  • progresses Python scalars in elementwise functions #807 in adding scalar support to element-wise functions. The changes are restricted to element-wise functions in order to simplify review.
  • updates the signatures for various dunder methods on the array object which did not indicate complex scalar support.
  • reformats and consolidates some of the notes included in affected function signatures to a dedicated "Notes" section, as done for special cases. This moves ancillary specification guidance to "below the fold". A subsequent PR will perform a similar clean-up for APIs outside of those affected by adding scalar support.

@kgryte kgryte added the API change Changes to existing functions or objects in the API. label Nov 25, 2024
@kgryte kgryte added this to the v2024 milestone Nov 25, 2024
@asmeurer
Copy link
Member

I think we need to update and reference this section for the specification on how scalars convert into arrays. https://data-apis.org/array-api/latest/API_specification/type_promotion.html#mixing-arrays-with-python-scalars

@asmeurer
Copy link
Member

And #841 if fixed will also potentially change how that text reads in relation to how real and complex scalar casting works.

"""


def hypot(x1: array, x2: array, /) -> array:
def hypot(x1: Union[array, float], x2: Union[array, float], /) -> array:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably int should also be allowed here to match https://data-apis.org/array-api/latest/API_specification/type_promotion.html#mixing-arrays-with-python-scalars

a Python int or float for real-valued floating-point array data types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've matched the text, where we say that x1 and x2 should have a real-valued floating-point data type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is something we should decide on. Should functions that operate on floating-point inputs accept int? If we are matching what we wrote for scalars with operators, then yes. If we want to be more strict, then no, but then we should probably update that section (which we need to do anyway so it applies to both operators and functions).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int should be accepted everywhere I think, like the link in the first comment here.

I think this has been decided on for us already by the language; int is being made more and more compatible with float, and Mypy et al. have decided that int is a subtype of float: https://stackoverflow.com/questions/59619778/mypy-why-is-int-a-subtype-of-float.

In practice it will also "just work" without changes, while trying to reject int inputs is cumbersome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and updated to enumerate explicit int support; however, two points:

  1. If int is already considered a float, it is not clear to me that saying Union[int, float] is even necessary. Simply because a type signature only includes float does not mean that downstream libraries would need to "reject" scalar values of type int. Meaning, the "just work" aspect should hold true whether or not we explicitly enumerate int as an accepted scalar type. Related: Clarify the float/int/complex special case python/typing#1746 and https://discuss.python.org/t/clarifying-the-float-int-complex-special-case/54018/30.
  2. I think that (1) is symptomatic of a larger issue which is the sometimes conflicting need to have the specification also serve the role of an authoritative type implementation. For several of the functions, the "spirit" of the specification is that, e.g., a function is intended to work with real-valued floating-point numbers. We then muddy the waters a bit by including explicit type annotations which reflect more an implementation detail than the intent, especially when providing an int may result in undefined behavior due to casting.

On the one hand having types in the signatures is nice for Sphinx and API docs; on the other hand, I see advantages to more clearly separating specification guidance from a concrete type implementation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

especially when providing an int may result in undefined behavior due to casting.

I can't think of a way that this could ever happen given our casting rules which give Python scalars lowest priority.

Copy link
Contributor Author

@kgryte kgryte Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The undefined/unspecified behavior would stem from cross-kind casting (e.g., an integer value which exceeds the maximum safe floating-point integer).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, that is a typically annoying corner case. I tend to say let's not care. This kind of corner case with super large numbers exists in every library, and it's invariably ignored with no real-world impact. I don't think it's a reason to say int is not supported. The usability downsides of not allowing kwarg=0 but requiring kwarg=0. seem much more important to me (in addition to what static typing thinks).

If int is already considered a float, it is not clear to me that saying Union[int, float] is even necessary.

My understanding is that we did this (certainly I have done this) to be very explicit, not because it matters for static typing.

@asmeurer
Copy link
Member

clip is already an elementwise function that supports scalars, but we should ensure the language is consistent.

@kgryte
Copy link
Contributor Author

kgryte commented Nov 28, 2024

In clip, which is slightly different in that the scalar arguments are optional, we already added guidance concerning differing data type kinds. However, we will still need to update to point to common documentation concerning scalar casting.

@ev-br
Copy link

ev-br commented Nov 28, 2024

Once python ints make the stage, I'd be ideal to explicitly spell the position on overflows. For both binary operators and elementwise functions. AFAICS, the spec only declares unspecified an attempt to cast a too-large python int to an integer dtype. However,

(Pdb) m32 = int(xp.finfo(xp.float32).max)
(Pdb) am32 = xp.asarray(m32, dtype=xp.float32)
(Pdb) am32
Array(3.4028235e+38, dtype=array_api_strict.float32)
(Pdb) am32 + m32
*** RuntimeWarning: overflow encountered in add
(Pdb) zzz = xp.asarray(m32*2, dtype=xp.float32)
*** RuntimeWarning: overflow encountered in cast

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

Successfully merging this pull request may close these issues.

4 participants