-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implementation of convolve #2205
base: master
Are you sure you want to change the base?
Conversation
84ad9a8
to
6d8689f
Compare
View rendered docs @ https://intelpython.github.io/dpnp/pull/2205/index.html |
6d8689f
to
c6dde99
Compare
5f429be
to
6eb94b2
Compare
c6dde99
to
b39e5c9
Compare
4168f27
to
3a03fdc
Compare
3a03fdc
to
1c52d67
Compare
View rendered docs @ https://intelpython.github.io/dpnp/pull/2205/index.html |
Array API standard conformance tests for dpnp=0.18.0dev0=py312he4f9c94_53 ran successfully. |
@antonwolfy please review |
) | ||
|
||
if a.ndim == 0: | ||
a = dpnp.reshape(a, (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.
not covered with tests
if a.ndim == 0: | ||
a = dpnp.reshape(a, (1,)) | ||
if v.ndim == 0: | ||
v = dpnp.reshape(v, (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.
not covered with tests
dpnp/tests/test_statistics.py
Outdated
if dtype == dpnp.bool: | ||
an = numpy.random.rand(a_size) > 0.9 | ||
vn = numpy.random.rand(v_size) > 0.9 | ||
else: | ||
an = (100 * numpy.random.rand(a_size)).astype(dtype) | ||
vn = (100 * numpy.random.rand(v_size)).astype(dtype) | ||
|
||
if dpnp.issubdtype(dtype, dpnp.complexfloating): | ||
an = an + 1j * (100 * numpy.random.rand(a_size)).astype(dtype) | ||
vn = vn + 1j * (100 * numpy.random.rand(v_size)).astype(dtype) |
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.
Can we use generate_random_numpy_array
from helper here?
dpnp.issubdtype(dtype, dpnp.integer) or dtype == dpnp.bool | ||
): | ||
# For 'direct' and 'auto' methods, we expect exact results for integer types | ||
assert_array_equal(result, expected) |
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 can we expect exact result for method="auto"
that might fall back on "fft"
but cannot expect it for "fft"
itself?
dpnp/tests/test_statistics.py
Outdated
if dpnp.issubdtype(rdtype, dpnp.integer): | ||
rdtype = dpnp.default_float_type(ad.device) |
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 do we need to change output dtypes (and also below at lines 202, and 223)? If numpy and dpnp outputs have different dtyps for a known reason, could not we use check_type
or check_only_type_kind
of assert_dtype_allclose
to handle it?
# For these outliers, the relative error can be significant. | ||
# We can tolerate a few such outliers. | ||
max_outliers = 8 if expected.size > 1 else 0 | ||
if invalid.sum() > max_outliers: |
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 not get the logic here! First, it seems the code never goes inside the if condition here which mean we do not compare the outputs at all. Second, as I understand it, invalid
is True when the difference between result and expected is large (outlier), so this condition indicates if there are more than 8 outliers the outputs should be compared. Shouldn't we avoid comparison when there is an outlier (or manually modify the values of outliers and then compare)?
Co-authored-by: Vahid Tavanashad <[email protected]>
Add implementation of
dpnp.convolve
. Implementation is mostly based on already existing functionality developed fordpnp.correlate
Similar to scipy.signal.convolve
method
keyword is introduced, but unlikescipy.signal.convolve
dpnp.convolve
works only with 1-d arrays.