[GSoC] Implemented convolution operation of two formal power series #17017

ArighnaIITG · 2019-06-12T15:08:33Z

References to other Issues or PRs

Brief description of what is fixed or changed

Convolution of two FormalPowerSeries objects have been implemented. Presently, it takes in a order term, and prints all the terms in the convoluted resultant fps upto order. No FormalPowerSeries object is being returned as of now.

For example --

>>> f1, f2 = fps(sin(x)), fps(exp(x))
>>> f1.convolute(f2, x, order=6)
x + x**2 + x**3/3 - x**5/12 + O(x**6)

I have only implemented linear convolution as of now. Whether other forms of convolution is required has to be discussed.

Other comments

Release Notes

series
- Implemented convolution operation of two formal power series in sympy.series.formal

sympy-bot · 2019-06-12T15:08:34Z

✅

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

series
- Implemented convolution operation of two formal power series in sympy.series.formal (#17017 by @ArighnaIITG)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

Convolution of two `FormalPowerSeries` objects have been implemented. Presently, it takes in a `order` term, and prints all the terms in the convoluted resultant `fps` upto `order`. No `FormalPowerSeries` object is being returned as of now.

For example --
```
>>> f1, f2 = fps(sin(x)), fps(exp(x))
>>> f1.convolute(f2, x, order=6)
x + x**2 + x**3/3 - x**5/12 + O(x**6)
```
I have only implemented `linear convolution` as of now. Whether other forms of convolution is required has to be discussed.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* series
   * Implemented convolution operation of two formal power series in `sympy.series.formal`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

smichr · 2019-06-12T15:32:53Z

sympy/series/formal.py

+        ==========
+
+        order : Number, optional
+            Specifies the order of the term upto which the polynomial should


Suggested change

Specifies the order of the term upto which the polynomial should

Specifies the order of the term up to which the polynomial should

jksuom · 2019-06-12T16:11:06Z

sympy/series/formal.py

@@ -1143,6 +1143,66 @@ def integrate(self, x=None, **kwargs):

        return self.func(f, self.x, self.x0, self.dir, (ak, self.xk, ind))

+    def convolve(self, other, x=None, order=4):
+        """ Convolute two Formal Power Series and return the truncated terms upto specified order.


Should this return the product of two power series? If so, then I think that the method should be renamed.

Yes currently it does that. But if we implement other types of convolution like dyadic or cyclic convolution, then they are no longer product of two power series. So I think, we should keep it convolute. I will try to extend the code to integrate cyclic, dyadic and other types of convolution into it.

Hi @ArighnaIITG ,

How do you plan to add new types of convolution in the future?

If they will be separate methods, I believe renaming it to product is good like @jksuom suggested.

or one way will be to allow a method kwarg, eg. f1.convolve(f2, method='linear').

ArighnaIITG · 2019-06-12T17:01:57Z

ping @leosartaj

sympy/series/formal.py

czgdp1807 · 2019-06-13T06:26:08Z

General comments on the way checks have been organized,

if isinstance(other, FormalPowerSeries):
            if self.dir != other.dir:
                raise ValueError("Both series should be calculated from the"
                                 " same direction.")
            elif self.x0 != other.x0:
                raise ValueError("Both series should be calculated about the"
                                 " same point.")
           <your-code>
else:
            raise ValueError("Both series should be an instance of FormalPowerSeries"
                             " class.")

can be reorganized for easy reading and better performance,

if not isinstance(other, FormalPowerSeries): # rejected at the first place, no futher processing needed. easy debugging and reading.
            raise ValueError("Both series should be an instance of FormalPowerSeries"
                             " class.")
if self.dir != other.dir:
                raise ValueError("Both series should be calculated from the"
                                 " same direction.")
elif self.x0 != other.x0:
                raise ValueError("Both series should be calculated about the"
                                 " same point.")
<your-code>

As far as I have observed all the checks are kept at the entry point of the method/function, in the decreasing order of priority.

sympy/series/formal.py

leosartaj · 2019-06-13T07:09:03Z

sympy/series/formal.py

@@ -1143,6 +1143,66 @@ def integrate(self, x=None, **kwargs):

        return self.func(f, self.x, self.x0, self.dir, (ak, self.xk, ind))

+    def convolve(self, other, x=None, order=4):
+        """ Convolute two Formal Power Series and return the truncated terms upto specified order.


Hi @ArighnaIITG ,

How do you plan to add new types of convolution in the future?

If they will be separate methods, I believe renaming it to product is good like @jksuom suggested.

or one way will be to allow a method kwarg, eg. f1.convolve(f2, method='linear').

leosartaj · 2019-06-13T07:09:39Z

sympy/series/formal.py

+
+        sympy.discrete.convolutions
+        """
+        from sympy.discrete.convolutions import convolution


Can this be top level import?

Just asking, can you please clarify on when the import should be top level and when not? Some conditions for this?

Just asking, can you please clarify on when the import should be top level and when not? Some conditions for this?

We try to follow the convention of imports at top-level only. It's easier to debug and read since you can see a list of everything up-top.

The only time I have ever added these imports inside of functions is when there are errors due to cyclic imports.

sympy/series/formal.py

sympy/series/tests/test_formal.py

leosartaj · 2019-06-13T07:30:05Z

@ArighnaIITG , please add what is left to be done in the PR (mark it as TODO).

codecov · 2019-06-15T04:44:45Z

Codecov Report

Merging #17017 into master will increase coverage by 0.086%.
The diff coverage is 84.615%.

@@              Coverage Diff              @@
##            master    #17017       +/-   ##
=============================================
+ Coverage   74.411%   74.498%   +0.086%     
=============================================
  Files          622       623        +1     
  Lines       160785    161362      +577     
  Branches     37738     37868      +130     
=============================================
+ Hits        119643    120212      +569     
- Misses       35818     35823        +5     
- Partials      5324      5327        +3

ArighnaIITG · 2019-06-15T04:46:39Z

I think there has been an issue with the branch. Several commits have been pushed here. @leosartaj can you guide me on this?

leosartaj · 2019-06-15T08:30:21Z

I think there has been an issue with the branch. Several commits have been pushed here. @leosartaj can you guide me on this?

You need to update your branch. Can you update your branch and then try to push.

Or also you can just get rid of merge commit and do a force commit.

sympy/series/formal.py

leosartaj · 2019-06-17T07:12:01Z

I think there has been an issue with the branch. Several commits have been pushed here. @leosartaj can you guide me on this?

You need to update your branch. Can you update your branch and then try to push.

Or also you can just get rid of merge commit and do a force commit.

ping @ArighnaIITG

ArighnaIITG · 2019-06-17T09:03:57Z

The branch has been correctly restored. I actually went on the wrong path, and my branch got messed up. You can review now. @leosartaj

ArighnaIITG · 2019-06-19T05:43:39Z

ping @leosartaj

sympy/series/formal.py

ArighnaIITG · 2019-06-22T04:59:31Z

@leosartaj , can you check what Travis build error this is and kindly notify me? I am unable to understand.

leosartaj · 2019-06-22T07:58:08Z

@leosartaj , can you check what Travis build error this is and kindly notify me? I am unable to understand.

It appears you're docstring for convolve is not correct.

This is the error from the log /home/travis/build/sympy/sympy/sympy/series/formal.py:docstring of sympy.series.formal.FormalPowerSeries.convolve:21:Definition list ends without a blank line; unexpected unindent.

You can try building the docs locally, to fix this.

jksuom · 2019-06-22T13:37:58Z

I would try making the "second level parameters" 'dyadic', etc. look different from the actual parameters by, for example, writing dyadic: instead of dyadic : .

ArighnaIITG · 2019-06-23T11:10:40Z

I would try making the "second level parameters" 'dyadic', etc. look different from the actual parameters by, for example, writing dyadic: instead of dyadic : .

Can you explain @jksuom? I couldn't catch you.

jksuom · 2019-06-23T11:26:40Z

It seems that the combination : (space-colon-space) has some special meaning for Sphinx. (Though I have not studied its code to understand what exactly.) Hence I would try to avoid that and omit the space between dyadic and :.

ArighnaIITG · 2019-06-24T13:32:03Z

@leosartaj I think this is ready for merging now.

leosartaj · 2019-06-26T16:44:16Z

Looks good to me. Will merge in a day if no objection.

jksuom · 2019-06-27T07:23:32Z

The name convolve does not look good to me. The only use of convolution in the theory of power series that I know of is the Cauchy product. The applied method is the linear convolution, with infinite cycle length. Other types of convolution, cyclic, dyadic, or subset convolution, have no use in power series AFAIK. Also the name "product" is probably more familiar to users of power series.

leosartaj · 2019-06-27T07:55:44Z

The name convolve does not look good to me. The only use of convolution in the theory of power series that I know of is the Cauchy product. The applied method is the linear convolution, with infinite cycle length. Other types of convolution, cyclic, dyadic, or subset convolution, have no use in power series AFAIK. Also the name "product" is probably more familiar to users of power series.

Thanks @jksuom for the review.

I feel that might be more appropriate for it. We should have operations only that make sense when dealing with power series.

Does it make sense, to have overloading for * ?

>>> f = fps(sin(x), x)
>>> g = fps(cos(x), x)
>>> f * g # is f.product(g)

@ArighnaIITG what are your thoughts on this?

jksuom · 2019-06-27T10:37:26Z

I think that overloading * is a natural idea. For its implementation, a minor modification of API would be desirable. convolve currently accepts an order parameter n that defaults to 6. For the product, the order should be derived from the factors. It could be the minimum of their orders.

ArighnaIITG · 2019-06-27T13:51:46Z

@leosartaj @jksuom I believe that all the various forms of convolutions are not really that applicable to formal power series, backed by the evidence that there is no prior literature describing these operations for power series. If that is the case, then I can think we can limit ourselves to linear and cyclic convolutions.
In that case, product will be better as the name of the function. I still think, we can use cyclic because Formal Power Series is after all a sequence of numbers.
I think the overloading part has already been cleared by @jksuom .

leosartaj · 2019-06-28T09:03:57Z

@leosartaj @jksuom I believe that all the various forms of convolutions are not really that applicable to formal power series, backed by the evidence that there is no prior literature describing these operations for power series. If that is the case, then I can think we can limit ourselves to linear and cyclic convolutions.
In that case, product will be better as the name of the function. I still think, we can use cyclic because Formal Power Series is after all a sequence of numbers.

Let's rename the function for now to product. Let's just keep the linear infinite cycle case. We can always extend later.

I think the overloading part has already been cleared by @jksuom .

For overloading let's wait until we have a class for this operation. It will be difficult to compute for minimum orders since by nature FormalPowerSeries objects are infinite. Once we have a class we can just start returning it's object when * operator is used.

leosartaj · 2019-07-01T10:40:45Z

Ping @ArighnaIITG

ArighnaIITG · 2019-07-01T19:17:55Z

Ping @leosartaj

leosartaj · 2019-07-04T07:10:15Z

This looks good to me. Merging. Thanks @ArighnaIITG for the work.

smichr reviewed Jun 12, 2019

View reviewed changes

ArighnaIITG added GSoC series labels Jun 12, 2019

jksuom reviewed Jun 12, 2019

View reviewed changes

czgdp1807 reviewed Jun 13, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

czgdp1807 reviewed Jun 13, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

czgdp1807 reviewed Jun 13, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

czgdp1807 reviewed Jun 13, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

leosartaj reviewed Jun 13, 2019

View reviewed changes

czgdp1807 mentioned this pull request Jun 14, 2019

Added cases in NDimArray operations for sparse array #17026

Merged

leosartaj reviewed Jun 15, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

ArighnaIITG added 2 commits June 17, 2019 14:15

Implemeted convolution operation

5e5c732

Refactored lists into sequences, and added tests and other changes

1a2b0a7

ArighnaIITG force-pushed the convolution_fps branch from 36c977f to 1a2b0a7 Compare June 17, 2019 08:46

Multiplied sequences instead of using SeqMul

b3616bd

ArighnaIITG changed the title ~~[GSoC] Implemeted convolution operation of two formal power series~~ [GSoC] Implemented convolution operation of two formal power series Jun 17, 2019

leosartaj reviewed Jun 19, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

leosartaj requested changes Jun 19, 2019

View reviewed changes

sympy/series/formal.py Outdated Show resolved Hide resolved

Changed the API of the convolution function

28e6970

ArighnaIITG added 2 commits June 24, 2019 17:25

Changed documentation

15a1817

Corrected trailing whitespace

8ed575b

leosartaj approved these changes Jun 26, 2019

View reviewed changes

leosartaj requested a review from jksuom June 26, 2019 16:44

leosartaj added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Jun 27, 2019

leosartaj self-requested a review June 27, 2019 07:56

Changed convolve to product

b91c21b

ArighnaIITG mentioned this pull request Jul 3, 2019

[GSoC] Introduction of FormalPowerSeries sub-classes, which is being returned by product, compose and inverse functions #17134

Merged

ArighnaIITG removed the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Jul 3, 2019

leosartaj approved these changes Jul 4, 2019

View reviewed changes

leosartaj merged commit 411757c into sympy:master Jul 4, 2019

leosartaj mentioned this pull request Jul 4, 2019

[GSoC] Implementation of composition and inversion of formal power series #17064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GSoC] Implemented convolution operation of two formal power series #17017

[GSoC] Implemented convolution operation of two formal power series #17017

ArighnaIITG commented Jun 12, 2019 •

edited

sympy-bot commented Jun 12, 2019 •

edited

smichr Jun 12, 2019

jksuom Jun 12, 2019

ArighnaIITG Jun 12, 2019

leosartaj Jun 13, 2019

ArighnaIITG commented Jun 12, 2019

czgdp1807 commented Jun 13, 2019 •

edited

leosartaj Jun 13, 2019

leosartaj Jun 13, 2019

czgdp1807 Jun 13, 2019

leosartaj Jun 13, 2019

leosartaj commented Jun 13, 2019

codecov bot commented Jun 15, 2019 •

edited

ArighnaIITG commented Jun 15, 2019

leosartaj commented Jun 15, 2019

leosartaj commented Jun 17, 2019

ArighnaIITG commented Jun 17, 2019

ArighnaIITG commented Jun 19, 2019

ArighnaIITG commented Jun 22, 2019 •

edited

leosartaj commented Jun 22, 2019

jksuom commented Jun 22, 2019

ArighnaIITG commented Jun 23, 2019 •

edited

jksuom commented Jun 23, 2019

ArighnaIITG commented Jun 24, 2019

leosartaj commented Jun 26, 2019

jksuom commented Jun 27, 2019

leosartaj commented Jun 27, 2019

jksuom commented Jun 27, 2019

ArighnaIITG commented Jun 27, 2019

leosartaj commented Jun 28, 2019

leosartaj commented Jul 1, 2019

ArighnaIITG commented Jul 1, 2019

leosartaj commented Jul 4, 2019

	Specifies the order of the term upto which the polynomial should
	Specifies the order of the term up to which the polynomial should

[GSoC] Implemented convolution operation of two formal power series #17017

[GSoC] Implemented convolution operation of two formal power series #17017

Conversation

ArighnaIITG commented Jun 12, 2019 • edited

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Jun 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArighnaIITG commented Jun 12, 2019

czgdp1807 commented Jun 13, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leosartaj commented Jun 13, 2019

codecov bot commented Jun 15, 2019 • edited

Codecov Report

ArighnaIITG commented Jun 15, 2019

leosartaj commented Jun 15, 2019

leosartaj commented Jun 17, 2019

ArighnaIITG commented Jun 17, 2019

ArighnaIITG commented Jun 19, 2019

ArighnaIITG commented Jun 22, 2019 • edited

leosartaj commented Jun 22, 2019

jksuom commented Jun 22, 2019

ArighnaIITG commented Jun 23, 2019 • edited

jksuom commented Jun 23, 2019

ArighnaIITG commented Jun 24, 2019

leosartaj commented Jun 26, 2019

jksuom commented Jun 27, 2019

leosartaj commented Jun 27, 2019

jksuom commented Jun 27, 2019

ArighnaIITG commented Jun 27, 2019

leosartaj commented Jun 28, 2019

leosartaj commented Jul 1, 2019

ArighnaIITG commented Jul 1, 2019

leosartaj commented Jul 4, 2019

ArighnaIITG commented Jun 12, 2019 •

edited

sympy-bot commented Jun 12, 2019 •

edited

czgdp1807 commented Jun 13, 2019 •

edited

codecov bot commented Jun 15, 2019 •

edited

ArighnaIITG commented Jun 22, 2019 •

edited

ArighnaIITG commented Jun 23, 2019 •

edited