Skip to content
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

WIP: Formal Power Series #7618

Closed
wants to merge 9 commits into from
Closed

WIP: Formal Power Series #7618

wants to merge 9 commits into from

Conversation

avichaldayal
Copy link
Contributor

Formal power series implementation using lazy recursion.

@skirpichev
Copy link
Contributor

In [1]: from sympy.series.formal import FormalSeries

In [2]: s1=FormalSeries(x, function=1/(1-x))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-31b2848ac013> in <module>()
----> 1 s1=FormalSeries(x, function=1/(1-x))

/home/sk/src/sympy/sympy/series/formal.py in FormalSeries(x, n, *args, **kwargs)
     49     function = kwargs.pop('function', None)
     50     if function:
---> 51         generator = findgen(x, function, k)
     52 
     53     c, e, k = generator

/home/sk/src/sympy/sympy/series/formal.py in findgen(x, function, k)
    217         diff = function.diff(x, order)
    218         if diff.is_rational_function():
--> 219             return findgen_rational(x, function)  # Integrate k times
    220 
    221     func = Function('f')

TypeError: findgen_rational() takes exactly 3 arguments (2 given)

return solveRE(x, RE, recurr, n, function)


class Stream(Expr):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should subclass Stream from Expr. The Stream is a container (just like Tuple) to represent a collections.abc.Iterator, maybe we should add also getitem interface too. You should also think about the efficiency. Do we want to recompute Stream again for first 10 terms?

Probably it's also not a right way to implement Stream's arithmetics (e.g. subclass from Expr and override add and so on). But keep going on as-is, lets build something working first, e.g. Stream(sin(x))*Stream(cos(x))...

val = [function.diff(x, i).subs(x, 0)/C.factorial(i) for i in range(0, len(terms)-1)]
init = dict(zip(key, val))

sol = rsolve(RE, r(k), init)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you can't use rsolve for all cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rsolve cannot solve some cases specially the hypergeometric type.
Ones that are of the form:- P * r(k+m) = Q * r(k)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Some cases" looks like you don't understand well which cases and what's wrong with rsolve.

No doubts - rsolve has algorithms to find solutions for such equations. Do we have a bug with some rsolve_* method or we indeed have a need to implement some new algorighm for rsolve? In the later case - we should improve the recurr module.

@skirpichev
Copy link
Contributor

Ok, I'm not going into the stuff to find generators, ODE's and so on. (Except for few minor remarks.)

  1. As I said before, Stream should be a basic container class, just like Tuple. No fancy arithmetic rules.
  2. Not sure which interfaces we want to implement for Stream, but at least getitem and iter.
  3. Every Basic instance should be rebuildable from its args: self = self.func(*self.args), see test_args.py should also test rebuilability #6426. And arguments should be of the Basic type. This doesn't holds in your case.
  4. Python is not a Scheme. You should consider to use generators:
    In [7]: sys.setrecursionlimit(200)
    In [8]: s1 = FormalSeries(x, function=sin(x))
    In [9]: s2 = FormalSeries(x, function=cos(x))
    In [10]: t = s1*s2
    In [12]: for i in xrange(150):
                    t=t.tail
    ...
    RuntimeError: maximum recursion depth exceeded

@avichaldayal
Copy link
Contributor Author

Stream should be a basic container class, just like Tuple.

Stream now inherits from Basic and acts as a basic container for generator objects. But it still violates the test_args rule. Its argument is a generator object which is not a Basic instance. Sympy should have something to make generators work with its codebase.

else:
raise TypeError("Invalid argument type")

def __add__(self, other):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, such methods shouldn't be here.

else:
raise TypeError("Invalid argument type")

def shift(self, n):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this interface? can't we just use getitem?

raise NotImplementedError('Cannot find simple differential equation for %s' % function)


def DEtoRE(DE, r, k):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can replace last two arguments with r(k) instance (AppliedUndef), like in the rsolve.

@skirpichev
Copy link
Contributor

Lets make something mergeable from this pr.

@avichaldayal, here is some todo:

  • First, please try to answer to the PR comments (except from Stream class, see below).
  • Then, you should clean up your tiny utillity functions. At least - we shouldn't do arbitrary and unconfigurable restrictions for the output (e.g. the order of ODE and so on).
  • As it seems we can't realize an approach, based on lazy streams - lets try FPS representation by formula for coefficients of the series. We have your findgen method for this. We also can implement some arithmetics (at least - addition/substraction, multiplication with the Cauchy product). This shouldn't be too hard.

@certik, @asmeurer - any thoughts?

@leosartaj
Copy link
Member

PR is stale. Features proposed already in master. Closing.

@leosartaj leosartaj closed this Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants