-
Notifications
You must be signed in to change notification settings - Fork 11
gh-519: test array API compliance of legacy mode functions #595
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
Conversation
467441d
to
7ab2fd8
Compare
The typing and rng issues need to be worked out ideally before merging this. Hence, marking as draft. |
7ab2fd8
to
e377ecf
Compare
49dac65
to
a084534
Compare
a084534
to
448e511
Compare
448e511
to
b41eb0f
Compare
Thanks @Saransh-cpp! What is the issue with |
3108e3b
to
e2265be
Compare
Thanks, @ntessore! |
e2265be
to
614b317
Compare
42746f1
to
2e05701
Compare
42904fe
to
c669fee
Compare
acc7d03
to
b3fa300
Compare
np.divide(a, norm, where=(norm > 0), out=np.zeros_like(a)), | ||
xp.divide(a, norm), |
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.
Just checking why dropping where
/out
?
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.
array_api_strict.divide
does not support those arguments. out
is not testing anything here and the where
argument is probably just reducing the number of computations (which is not very large in this case).
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.
norm > 0
is necessary if dealing with rank-deficient matrices, e.g. to get the correlation matrix of this covariance matrix:
To be fair, I don't we are testing that case currently
4174e4a
to
56a8d44
Compare
) | ||
|
||
# cannot deal with negative variances | ||
with pytest.raises(ValueError, match="negative values"): | ||
glass.algorithm.cov_nearest(np.diag([1, 1, -1])) | ||
glass.algorithm.cov_nearest(xp.asarray([[1, 0, 0], [0, 1, 0], [0, 0, -1]])) |
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.
There is no diag
function or alternative in array API standard.
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.
Given the size of this, that doesn't seem like a problem really
np.divide(a, norm, where=(norm > 0), out=np.zeros_like(a)), | ||
xp.divide(a, norm), |
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.
array_api_strict.divide
does not support those arguments. out
is not testing anything here and the where
argument is probably just reducing the number of computations (which is not very large in this case).
28fad94
to
5480683
Compare
This should finally be ready now, @paddyroddy @ntessore! |
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 looks really clean 👌
Merging this so that it does not go stale again! |
Description
Add array API tests for legacy mode functions added in
glass.algorithm
. The functions added in other modules do not fully support the array API standard; therefore, those functions can be dealt with once we add full support.Closes: #519
Checks