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

fix: clarify that condition should have a boolean dtype in where #868

Merged
merged 3 commits into from
Dec 12, 2024

Conversation

lucascolley
Copy link
Contributor

@lucascolley lucascolley commented Dec 6, 2024

Towards data-apis/array-api-extra#49, scipy/scipy#21783

gh-116 removed this detail because it was contained in a type annotation. Unfortunately, that detail hasn't made its way back into the spec yet, which is causing trouble downstream, with array-api-strict supporting non-boolean input, but torch from array-api-compat only supporting boolean input.

cc @ev-br @asmeurer

@ev-br
Copy link

ev-br commented Dec 10, 2024

Would be nice to add to the agenda of the consortium meeting on Thursday. From the spec I cannot figure out if it is meant to be truly boolean or just truth/falsy (and if so, torch is not conformant)

@lucascolley
Copy link
Contributor Author

from the spec I cannot figure out if it is meant to be truly boolean or just truth/falsy (and if so, torch is not conformant)

Ralf said:

+1 for accepting only boolean arrays. The note in the numpy.where docstring recommends doing that as the more correct option, and it's easy to do

And indeed this used to be in the standard, until it was (seemingly accidentally) lost with gh-116, as mentioned above.

@rgommers rgommers added this to the v2024 milestone Dec 11, 2024
@rgommers rgommers added the Backport Changes involve backporting to previous versions. label Dec 11, 2024
@rgommers
Copy link
Member

And indeed this used to be in the standard, until it was (seemingly accidentally) lost with gh-116, as mentioned above.

That indeed seems to be completely accidental. I marked this for the 2024 milestone and backporting, will merge soon unless there are more comments, since it looks uncontroversial and caused by a minor editing hiccup only.

@kgryte
Copy link
Contributor

kgryte commented Dec 12, 2024

Since this should be backported, I believe we can go ahead and do that in this PR.

@kgryte kgryte changed the title bug: where: clarify that condition should have a boolean dtype fix: clarify that condition should have a boolean dtype in where Dec 12, 2024
@kgryte kgryte added the Needs Changes Pull request which needs changes before being merged. label Dec 12, 2024
@kgryte kgryte added Needs Review Pull request which needs review. and removed Needs Changes Pull request which needs changes before being merged. labels Dec 12, 2024
Copy link
Contributor

@kgryte kgryte left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, @lucascolley!

@kgryte kgryte removed the Needs Review Pull request which needs review. label Dec 12, 2024
@kgryte
Copy link
Contributor

kgryte commented Dec 12, 2024

As the language uses should, not must, this allows array libraries to support truthy and falsy conditions for reasons of backward compat, and given that this was an editorial oversight, I'll go ahead and merge.

@kgryte kgryte merged commit f7d16ff into data-apis:main Dec 12, 2024
3 checks passed
@lucascolley
Copy link
Contributor Author

data-apis/array-api-strict#106 implements this in xp-strict

@seberg
Copy link
Contributor

seberg commented Dec 12, 2024

I don't care for the churn, but one day it may be nice spell things always clearly from either implementation or user perspective.

The user must pass a boolean (and the implemntation must support it). For the implementation you could suggest that it should reject non-bools if you are opinionated.
(Although, the may support more should be an implicit rule everywhere unless stated otherwise as a "must raise if ...".)

@lucascolley lucascolley deleted the where-bool branch December 12, 2024 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport Changes involve backporting to previous versions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants