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

Allow Python scalars in array_namespace #147

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

asmeurer
Copy link
Member

They are just ignored. This makes array_namespace easier to use for functions that accept either arrays or scalars.

I'm not sure if I should have this behavior by default, or if it should be enabled by a flag.

They are just ignored. This makes array_namespace easier to use for functions
that accept either arrays or scalars.

I'm not sure if I should have this behavior by default, or if it should be
enabled by a flag.
@asmeurer
Copy link
Member Author

We can also automatically ignore None inputs. The scipy helper does this so it would clearly be useful.

@lucascolley
Copy link
Contributor

lucascolley commented Jun 28, 2024

I would say this should be opt-in, given the standard really focusses on just arrays. In practice, it will probably be used more often than not (at least for now), but given that (for now) most projects will wrap array_namespace, it isn't a problem to have the extra argument.

In SciPy, it doesn't really matter either way as we coerce scalars to NumPy arrays before passing them to array_namespace.

@asmeurer
Copy link
Member Author

There are some active discussions on allowing scalars in more places in the array API. See data-apis/array-api#805 and data-apis/array-api#807.

but given that (for now) most projects will wrap array_namespace, it isn't a problem to have the extra argument.

I'm trying to make that not the case. Some projects like SciPy will have specific reasons they'll need to wrap things, but ideally whatever most projects need should be implemented already in the wrappers here.

@lucascolley
Copy link
Contributor

I think another form of wrapping that will be very common is using a default backend when all arguments of array_namespace are Python scalars.

@asmeurer
Copy link
Member Author

asmeurer commented Jul 1, 2024

That would be easy to add. I would caution against it though unless you are doing it for backwards compatibility. If you default to NumPy, for instance, you could end up in a situation where a user gets a NumPy array from one function because they passed in scalars, and another array from another function because they passed in arrays.

@lucascolley
Copy link
Contributor

Yeah, I'm thinking that the reason would be to maintain BC

@ev-br
Copy link
Contributor

ev-br commented Nov 15, 2024

From experience adding array api support to scipy.ndimage and scipy.signal:

  • ignoring Nones is a big +1. As mentioned, scipy has a helper anyway, and it does not seem risky
  • ignoring python scalars would be nice actually. scipy has one-off helpers, which basically replace a scalar with an int a None, cf https://github.com/scipy/scipy/blob/main/scipy/ndimage/_delegators.py#L201 ; This is not a deal-breaker though, because scipy needs other helpers, too (_skip_if_dtype etc).
  • adding logic of the sort "assume numpy if all scalars" does not look desirable. "In the face of ambiguity" and all that.

In general, I'd say the logic for unknown arguments (None, possibly scalars) should be "ignore an unknown object and deduce the namespace from all other arguments". Scipy does it in a couple of places, e.g. there's a scipy PR which makes xp_assert_close(array, list) convert the list to the namespace of array.

In that view, ignoring scalars is fine as long as array_namespace() raises on no inputs:

In [5]: xp.array_namespace()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [5], in <cell line: 1>()
----> 1 xp.array_namespace()

File ~/repos/array-api-compat/array_api_compat/common/_helpers.py:563, in array_namespace(api_version, use_compat, *xs)
    560         raise TypeError(f"{type(x).__name__} is not a supported array type")
    562 if not namespaces:
--> 563     raise TypeError("Unrecognized array input")
    565 if len(namespaces) != 1:
    566     raise TypeError(f"Multiple namespaces for array inputs: {namespaces}")

TypeError: Unrecognized array input

@asmeurer
Copy link
Member Author

That's very useful feedback. It sounds like the behavior in this PR matches what you are suggesting (it does indeed raise if inputs are only scalars or None, as you can see from the tests).

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.

@ev-br
Copy link
Contributor

ev-br commented Nov 16, 2024

Tried it locally and it does work as expected. Would be great to be able to use this downstream. So, for what it's worth, LGTM.

@asmeurer asmeurer merged commit 8d3f5d5 into data-apis:main Nov 18, 2024
39 checks passed
@asmeurer
Copy link
Member Author

Looks like this broke some of the tests. We should have merged the latest main before merging here since it was an older PR.

@asmeurer
Copy link
Member Author

This breaks NumPy scalar support because of this annoying fact:

>>> isinstance(np.float64(0), float)
True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants