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
ENH: constants: add array api support #20593
Conversation
@lucascolley I was a bit confused about how to adapt the type hinting, any suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you've done so far is good, but the tests need to check that the helper functions work with xp
arrays. Currently, the xp
parameter is just unused. The inputs should be xp
arrays, and so should the expected values. That should confirm whether the correct namespaces are returned.
EDIT: you may want to keep a test / add cases for Python list input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I'd try to help here, but my main comment is the same as what @lucascolley wrote about the tests.
# Convert from `old_scale` to Kelvin | ||
if old_scale.lower() in ['celsius', 'c']: | ||
tempo = np.asanyarray(val) + zero_Celsius |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will use of asarray
instead of asanyarray
break anything that is supposed to be supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://stackoverflow.com/questions/59350359/any-examples-for-numpy-asanyarray-vs-asarray only if you are using subclasses of an ndarray
, hopefully that is niche enough not to be an issue? This looks like a quirk of history rather than a change to in response to something 52f43e9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth trying to find the niche case - the alternative would be val = np.asanyarray(val) if is_numpy(xp) else xp.asarray(val)
I think. array_namespace
returns a numpy
namespace for anything unrecognised but coercible by np.asanyarray
(that isn't otherwise refused).
Dropping it should probably be an explicit deprecation otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we add a subok
argument to _asarray
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping it should probably be an explicit deprecation otherwise.
If it's advertised, sure, but I don't think it's needed if subclasses were supported by accident.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think it's a good idea to clarify the documentation, maybe as we go through these, to specify:
array
(if that's how array-API compatible arrays are to be identified in documentation, following the examples of the array API standard docs, e.g.abs
)- dimensionality
- dtype
For example, n-D real floating array
. But there would need to be a standard language for that, and I don't know that one exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I've written somewhere about this before but not sure where. There are some orthogonal issues:
- the 48 occurrences of
np.asanyarray
should probably be replaced withnp.asarray
unless there really are documented special cases. This may or may not require deprecations, depending on how much the behaviour can be deemed undocumented. - parameters are variably described as
ndarray
orarray_like
, but in both cases it usually means 'anything thatnp.asarray
can make sense of'. This should be replaced with something likearray
as you suggest, along with the other information. - We need to keep supporting array-likes with
np
arrays, but they will not be supported for other backends. Hence, there is even more reason to get away from sayingarray_like
. We can document elsewhere that array-likes will still be supported with NumPy as the assumed backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ndarray subclass support is not in great/consistent shape, but I also don't see the need to break it if a one-liner keeps things unchanged.
This is covered in gh-18286:
Input type | Return type | When | Behavior | Notes |
---|---|---|---|---|
numpy.ndarray subclasses | same subclass type | always | so always use np.asanyarray checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about following Jake's suggestion with _asarray(x, ..., xp=None, ..., sub_ok=True)
which uses np.asanyarray
instead of np.asarray
for NumPy, but still xp.asarray
for alternative backends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me.
Thanks both, I have updated the tests |
ca5ba6c
to
0b61cb7
Compare
0b61cb7
to
1368845
Compare
Looks like a tolerance bump might be required? |
Re: tolerance bump, what do you think of changing the default scipy/scipy/_lib/_array_api.py Lines 286 to 299 in 5e1cb66
Effectively, the default tolerance is a little tighter for |
Wait wasn't there some open PR to refactor the whole module? IIRC the main maintainer of this module (not a SciPy maintainer) has made a few PRs for that. |
#20430 is the last one. But they have a few of these. |
Yes, I have seen these but it is quite unclear which are ready for review. Many have major BC breaks too. I'm not sure they block this either as most seem to be to do with the constants themselves, not these functions |
I don't know either, but I think it would be good to clarify. It would be a shame if we had to turn down some already done work because we would have this go in instead. Though if this PR takes into account the new proposals then all good. I just wanted to be sure you all had that other person in mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once my one comment about testing is addressed, and we have checked that we are happy with the np.asanyarray
removal, and the static typing.
EDIT: and yes, this PR shouldn't interfere with the other constants PRs
fyi, I didn't see any additional failures when testing this branch with GPU backends (CuPy, torch). |
ce7b574
to
ed91205
Compare
ed91205
to
d566ff1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was an easy one to play around with. Happy to close if we don't want this!
Thanks @j-bowhay! I think we do want this PR, as it can be useful and it's a small diff to add array API support.
My main comment is to keep the diff as small as possible, and keep it as focused on adding array API support and tests. In particular:
- don't change return types for
numpy.ndarray
subclasses - don't change type annotations
- don't remove
array_like
from docs
EDIT: and yes, this PR shouldn't interfere with the other constants PRs
Agreed with comments on these. It's good to think about it, however this diff is small enough that it should not impact other long-running PRs.
skip_xp_backends = pytest.mark.skip_xp_backends | ||
|
||
|
||
class TestConvertTemperature: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already had to touch all lines here so grouping the tests does not really increase the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the static typing is resolved!
[skip ci] Co-authored-by: Lucas Colley <lucas.colley8@gmail.com>
[skip cirrus] [skip circle]
Thanks for the fix @rgommers I think this is good from my end now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, everyone and CI are happy - let's give this a go:) Thanks @j-bowhay & reviewers!
Reference issue
towards #18867
What does this implement/fix?
Adds array API support to the three functions in
constants
Additional information
Appreciate this is probably the least important module to add support for, however, it was an easy one to play around with. Happy to close if we don't want this!