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

Implemented convolution using Walsh Hadamard Transform #14783

Merged
merged 3 commits into from Jun 11, 2018

Conversation

Projects
None yet
2 participants
@sidhantnagpal
Member

sidhantnagpal commented Jun 9, 2018

Brief description of what is fixed or changed

  1. Added convolution using Walsh Hadamard Transform
  2. Included convolution_fwht in convolution method

Other comments

@Abdullahjavednesar Abdullahjavednesar requested a review from jksuom Jun 10, 2018

raises(TypeError, lambda: convolution(b, d, fft=True, dps=2, dyadic=True))
raises(TypeError, lambda: convolution(b, d, ntt=True, prime=p, dyadic=True))
# fwht is a specialized variant of fft, TypeError should not be raised
assert convolution(b, d, fft=True, dyadic=True) == convolution_fwht(b, d)

This comment has been minimized.

@jksuom

jksuom Jun 11, 2018

Member

This might be confusing to some users. The implementations fft and fwht are similar, but they are applied in different type of convolutions (linear and dyadic). I am not sure that they should be accepted together.

@jksuom

This comment has been minimized.

Member

jksuom commented Jun 11, 2018

Thanks, I think this is ready.

@jksuom jksuom merged commit 19e7f91 into sympy:master Jun 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment