Skip to content

Add signbit specification for determining whether a sign bit is set #705

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

Merged
merged 3 commits into from
Dec 2, 2023

Conversation

kgryte
Copy link
Contributor

@kgryte kgryte commented Nov 15, 2023

This PR

  • resolves Need for signbit in the array API spec #670 by adding a specification for signbit for determining whether a sign bit is set for each floating-point number in a provided input array.
  • follows Python conventions in which a boolean is returned. In C99, this is a macro which returns an integral value, where a positive result can be any non-zero integral value and a negative result must be zero.
  • limits the input data type to real-valued floating-point data types. The current text uses "should", indicating that array libraries may extend support to additional data types (e.g., signed integer data types) for, e.g., backward compatibility reasons. This PR does not use the more liberal real-valued data type, as the intent of this function is for distinguishing +0 and -0 and the determining the sign of NaN, values which are not possible outside of floating-point data types. For complex data types, users need to provide the real and imaginary components separately.
  • as this API is commonly implemented across array libraries without deviation, there are no open questions requiring resolution before specification inclusion.

Verified

This commit was signed with the committer’s verified signature.
kgryte Athan
@kgryte kgryte added the API extension Adds new functions or objects to the API. label Nov 15, 2023
@kgryte kgryte added this to the v2023 milestone Nov 15, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM overall, thanks @kgryte. One sentence I'd add to the returns section is that for negative values the result is expected to be True, and for positive values False. That won't be completely obvious otherwise to readers not familiar with the low-level details.

Copy link
Contributor

@leofang leofang left a comment

Choose a reason for hiding this comment

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

I assume input shape-preserving is an implicit assumption, given that signbit just follows other elementwise docs?

@kgryte
Copy link
Contributor Author

kgryte commented Nov 30, 2023

One sentence I'd add to the returns section is that for negative values the result is expected to be True, and for positive values False. That won't be completely obvious otherwise to readers not familiar with the low-level details.

@rgommers Isn't that already covered in the special cases section?

    - If ``x_i`` is a positive (i.e., greater than ``0``) finite number, the result is ``False``.
    - If ``x_i`` is a negative (i.e., less than ``0``) finite number, the result is ``True``.

Ref: https://github.com/data-apis/array-api/pull/705/files#diff-153d068dcea18980d39e9465019663509adce3ee4842a48738b37a2a64c693f5R2244-R2245

@kgryte
Copy link
Contributor Author

kgryte commented Nov 30, 2023

I assume input shape-preserving is an implicit assumption, given that signbit just follows other elementwise docs?

@leofang That is correct. We don't explicitly mention the expected output shape across the element-wise specifications, but we should probably be explicit. I can submit a follow-up PR to address this more broadly.

@rgommers
Copy link
Member

Isn't that already covered in the special cases section?

No, special cases are special for a reason, and should not be needed to deduce the main behavior of a function. The text always has to make sense if you'd simply delete the special cases section.

@kgryte
Copy link
Contributor Author

kgryte commented Nov 30, 2023

The text always has to make sense if you'd simply delete the special cases section.

IMO this is splitting hairs. The main behavior is precisely as described, as the API should not be confused with the signum function. Meaning, using signbit to test less than or greater than zero is likely an anti-pattern; one can use one of sign, less, or greater instead. Based on a survey of docs,

  • PyTorch does not include the requested language.
  • NumPy uses misleading language. A value does not have to be less than zero in order for the sign bit to be set.
  • CuPy copies over the same misleading description from NumPy.
  • Dask copies over the same misleading description from NumPy.
  • JAX copies over the same misleading description from NumPy.

@rgommers
Copy link
Member

It is not a big ask to add something like: "if the sign bit of a value is set, i.e. has value 1 or the value has a minus sign, the result will be False". Or add the math expression like the sign function (x / |x|) for clarity. I didn't ask for fun, I needed to actually double check when I first read it.

Verified

This commit was signed with the committer’s verified signature.
kgryte Athan

Verified

This commit was signed with the committer’s verified signature.
kgryte Athan
@kgryte
Copy link
Contributor Author

kgryte commented Nov 30, 2023

@rgommers I've added an extended description.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @kgryte, LGTM now.

@rgommers
Copy link
Member

rgommers commented Dec 2, 2023

Two approvals and everyone was happy with this addition, so I'll hit the green button here.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need for signbit in the array API spec
3 participants