-
Notifications
You must be signed in to change notification settings - Fork 21
Explicit shape comparison for dpnp
and numpy
outputs
#2295
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
base: master
Are you sure you want to change the base?
Conversation
View rendered docs @ https://intelpython.github.io/dpnp/pull/2295/index.html |
Array API standard conformance tests for dpnp=0.18.0dev1=py312he4f9c94_17 ran successfully. |
dpnp
and numpy
outputs
2badc2f
to
135df75
Compare
a47ee75
to
94c792e
Compare
dpnp/dpnp_iface_linearalgebra.py
Outdated
We need to adjust the behavior of the multiply function when it is | ||
used for special cases of scalar-array dots. |
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 is a bit unclear what kind of adjustment is implementing. To have the same dtype as numpy, i.e. dpnp.dot(a, sc).dtype == dpnp.float64
, or what?
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.
Do we need to state that change in the changelog?
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.
Updated the explanation and added an entry to change log. Note that in this PR, only the behavior of dpnp.outer
is changed because of using this function. For other functions that fall back on _call_multiply
nothing is changed in this PR.
@@ -292,7 +292,7 @@ def test_indexing_array_negative_strides(self): | |||
|
|||
slices = (slice(None), dpnp.array([0, 1, 2, 3])) | |||
arr[slices] = 10 | |||
assert_array_equal(arr, 10.0) | |||
assert_equal(arr, 10.0, strict=False) |
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.
Why don't we need strict
here?
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 is comparing an array with a scalar, if strict is True, shape comparison will fail.
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.
But I thought in that case we will check that dpnp has shape () and it will not raise any exception.
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.
The logic that is currently used is for comparing shape of the two arrays and the exception is for a case when dpnp return a 0D array and numpy return a scalar, for which, the code validates dpnp output is indeed a 0D array.
We do not have any check-point to avoid shape comparison of a non-0D dpnp array (here the array has a shape of (4, 4)) and a scalar.
In the test suite, the result arrays from
dpnp
andnumpy
were compared, but their shapes were not explicitly checked for equality. As a result, tests could pass even if the shapes differed, such as one being(1,)
and the other()
.This PR adds an explicit check to ensure that the output shapes of
numpy
anddpnp
match. Additionally, tests have been updated accordingly to pass with this new check.