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

Added convolution module #14745

Merged
merged 9 commits into from May 30, 2018

Conversation

Projects
None yet
4 participants
@sidhantnagpal
Member

sidhantnagpal commented May 25, 2018

Brief description of what is fixed or changed

The following changes will be covered in this PR:

  • Add convolution module
  • convolution_fft method
  • convolution_ntt method
  • convolution method (general)
  • Support cyclic convolution

Other comments

sidhantnagpal added some commits May 25, 2018

@@ -115,7 +119,7 @@ def convolution_fft(a, b, dps=None):
a = ifft(a, dps)[:m]
return a
return a if c is None else [sum(a[i::c]) for i in range(c)]

This comment has been minimized.

@jksuom

jksuom May 28, 2018

Member

This operation is essentially independent of implementation. Could this be handled by the main function convolution? It would not be necessary to pass cycle to implementation functions like convolution_fft and convolution_ntt.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 28, 2018

Member

Yes, that could be done, but the specialized functions are also public - wouldn't they be requiring an extra argument for cyclic convolution?

This comment has been minimized.

@jksuom

jksuom May 28, 2018

Member

They can be public but I think that they can be specialized to a single operation. convolution should be the function that most users would need.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 28, 2018

Member

Okay, so cyclic should be available only to convolution?

This comment has been minimized.

@jksuom

jksuom May 28, 2018

Member

I think that that would suffice, at least for now.

sidhantnagpal added some commits May 28, 2018

Recurrence Evaluation - reval_hcc
"""
from .transforms import (fft, ifft, ntt, intt)
from .convolution import (convolution, convolution_fft, convolution_ntt)

This comment has been minimized.

@jksuom

jksuom May 28, 2018

Member

I think that it would suffice to import only convolution here in __init__.py. A user that would want specialized functions could import them by from sympy.discrete.convolution import *. In other words, they can be public but not actively exported.

This comment has been minimized.

@sidhantnagpal
dps = hints.pop('dps', None)
p = hints.pop('prime', None)
c = hints.pop('cycle', None)

This comment has been minimized.

@jksuom

jksuom May 30, 2018

Member

Here 0 could be the default instead of None

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 30, 2018

Member

Okay, so should cycle=None raise a ValueError?

This comment has been minimized.

@jksuom

jksuom May 30, 2018

Member

I think it could. There should be no reason for using None. (0 need not be given, either, but that could be accepted.)

This comment has been minimized.

@jksuom

jksuom May 30, 2018

Member

We can expect that if a user sets cycle then it would be a positive integer though 0 need not raise an exception.

@jksuom

This comment has been minimized.

Member

jksuom commented May 30, 2018

Thanks, this looks good to me.

@jksuom jksuom merged commit 852ceb0 into sympy:master May 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
a, b : iterables
The sequences for which convolution is performed.
hints : dict

This comment has been minimized.

@asmeurer

asmeurer Jul 12, 2018

Member

hints isn't really a dict from the user point of view. Rather, there are keyword arguments. So it would be better to just list the keyword arguments on the function signature directly (**kwargs should only be used when arbitrary keyword arguments are required).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment