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: stats.circ___: add array-API support #20595
Conversation
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.
Some questions and comments.
This is getting a little out of hand with lint complaining about |
Do you have the same ruff version locally (0.4.2)? |
Probably not. I thought we would have configured a rule set, though. I guess as ruff has added these, and we don't exclude them. |
We enabled all UP rules in gh-19516. UP031 should be in any compatible version of ruff that you have locally, as our minimum is 0.0.292, and the rule was added in 0.0.229. I would guess that it is due to detection differences over versions of ruff due to bug fixes. |
I think the regular CI is already picking some of them up, but here are fails I see locally with GPU:
|
Interesting. No, I don't think it is picking up in CI. I am running with CuPy locally, and I wasn't getting those. |
Thanks @tylerjereddy. Can you give the full failure information (e.g. actual relative error)? Also, can you reproduce these two:
I can't reproduce them on this machine. |
[skip cirrus] [skip circle]
I can't reproduce those two either. The PR seems fine on CPU, I'll try to add some inline comments re: the failure details on GPU. |
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.
OK, I'm happy with this now. aside from the comments here and the one failure, which I can't reproduce.
=================================== FAILURES ===================================
__________________ TestCircFuncs.test_circfuncs_small[torch] ___________________
scipy/stats/tests/test_morestats.py:2479: in test_circfuncs_small
V1 = xp_test.var(x*xp.pi/180, correction=xp.asarray(0.0))
M1 = tensor(19.9571, dtype=torch.float64)
M2 = tensor(19.9571, dtype=torch.float64)
self = <scipy.stats.tests.test_morestats.TestCircFuncs object at 0x7f625bdd31d0>
x = tensor([20.0000, 21.0000, 22.0000, 18.0000, 19.0000, 20.5000, 19.2000],
dtype=torch.float64)
xp = <module 'torch' from '/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/torch/__init__.py'>
xp_test = <module 'scipy._lib.array_api_compat.torch' from '/home/runner/work/scipy/scipy/build-install/lib/python3.11/site-packages/scipy/_lib/array_api_compat/torch/__init__.py'>
scipy/_lib/array_api_compat/torch/_aliases.py:425: in var
res = torch.var(x, tuple(range(x.ndim)), correction=correction, **kwargs)
E TypeError: var() received an invalid combination of arguments - got (Tensor, tuple, correction=Tensor), but expected one of:
E * (Tensor input, tuple of ints dim, bool unbiased, bool keepdim, *, Tensor out)
E * (Tensor input, tuple of ints dim, *, int correction, bool keepdim, Tensor out)
E * (Tensor input, bool unbiased)
E * (Tensor input, tuple of names dim, bool unbiased, bool keepdim, *, Tensor out)
E * (Tensor input, tuple of names dim, *, int correction, bool keepdim, Tensor out)
axis = None
correction = tensor(0.)
keepdims = False
x = tensor([0.3491, 0.3665, 0.3840, 0.3142, 0.3316, 0.3578, 0.3351],
dtype=torch.float64)
The whole point of calling array_namespace
is to get an array-API xp_test
so that it has var
. Clearly x
is a tensor. Technically, correction
can just be an int
or float
, but I think I've tried that, in addition to leaving it at its default value. I would think it's a bug in array_api_compat
, but this is using SciPy's vendored version, right?
Well, the message suggests that it should be an int
, whereas the default is a 0.0
, so let's try that.
Yeah we must have an older version of torch that isn't perfectly compatible with our array-api-compat.
@tylerjereddy please run these tests once more with TF GPU when you have a moment. Thanks! |
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.
This is looking good, some minor comments, most of which are for my own understanding
So, you're at parity with |
Gentle reminder that these should be gone with gh-19900 |
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.
OK If tests pass, I think I'm happy with this, and I think comments have been addressed.
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.
One query but otherwise this is good to go
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.
Thanks @mdhaber LGTM
Reference issue
gh-20544
What does this implement/fix?
Makes
circmean
,circstd
, andcircvar
array-API compatible.