Implementing Sequences #9435

leosartaj · 2015-05-27T19:08:28Z

A Sequence is finite or infinite lazily evaluated list. Coefficients will only be evaluated when required.

TODO

toolforger · 2015-05-27T19:42:00Z

Some quick bikeshedding: "Sequence" is a good name for what you're doing, but the term is already too overloaded with other, inconsistent meanings. I'd stick with "LazyList" or something like that.
I'm not going to insist on that though.

My knee-jerk reaction was "is it using the standard mechanisms in Python, like Iterators?"
Then I saw it's using generators.
I'd like somebody with a higher Python Iterators belt than mine check whether this is typical Python idioms. Not because that's better or anything, but because that would make make it easier for future maintainers if it's following typical Python patterns.

leosartaj · 2015-05-27T19:46:03Z

It's part of my GSoC project, some more details. Well, I will think on the name though. It can be confusing.

toolforger · 2015-05-27T20:12:31Z

It's part of my GSoC project, some more [details](https://github.com/sympy/sympy/wiki/GSoC-2015-Application-Sartaj-Singh:-Improving-the-series-package-and-limits-in-SymPy).

Ah I see. Laziness is a pretty big thing, as it can have counterintuitive effects. For example, it allows infinite lists, which sounds neat until you realize that comparing infinite lists for equality is undecidable. From SymPy's perspective, undecidability isn't a big issue, we need to deal with it anyway. Usually we go the easy route: punt, abort whatever we're doing, and leave the expression unchanged, try a different strategy, or do whatever else is the route of least damage. If your code already does equality testing on infinite lists, you'll need to guard against infinite iteration/recursion when iterating over the elements of a series.

jcrist · 2015-05-28T01:01:25Z

sympy/series/__init__.py


 O = Order
-
-__all__ = ['gruntz', 'limit', 'series', 'O', 'Order', 'Limit', "residue"]


Why did you delete this, instead of adding your stuff to it? This controls what's actually imported from the submodules.

Oh, a mistake on my part. Will fix this in my next commit.

jcrist · 2015-05-28T01:48:00Z

Looks like a good start! I think you need to think about the design of these classes a little bit more. Specific things:

What should be allowed in args for each type of Sequence. How should these args be ordered? Note that all args should (ideally) be sympy objects, and be hashable.
What inputs should be allowed for each type of Sequence?
What validation needs to be done for each type of Sequence to ensure no invalid state is possible?
How can we share as much functionality between these classes as possible?

leosartaj · 2015-05-28T11:38:33Z

I have followed the design, that for every sequence, there will be first a generator(generates the sequence) and second the interval. Do you see any problems with this design? Well, I have tried to stick with all args being Sympy objects.
Each sequence allows a generator and an interval only. Generator can be different for different sequences. Though, interval has a consistent structure throughout.
I will work more on the validation part.
For this purpose, I decided to create SeqExpr, so that common stuff can be abstracted out.

flacjacket · 2015-05-28T19:15:43Z

sympy/series/sequences.py

+        return self._eval_coeff(i)
+
+    def _eval_coeff(self, i):
+        raise NotImplementedError(filldedent(""" The _eval_coeff method should\


As a side note, you can do something like:

some_func("This is a long " "wrapping string that " "Python will put together correctly")

so you don't have to deal with dedenting long text

Other small changes: * Got rid of _foo properties (sympy#9435 (comment)) * Other minor changes (Typo and such)

leosartaj · 2015-05-28T21:08:43Z

On the basis of suggestions by @jcrist @flacjacket

Changes:

Implemented a wrapper function that returns appropriate sequence.
Improved validation of arguments (hopefully)
Got rid of _foo properties
Minor changes(typo and such)

leosartaj · 2015-05-28T21:11:03Z

I have started working on operations now (starting with addition).

leosartaj · 2015-05-30T00:11:24Z

I have implemented SeqAdd class. Addition is now supported. Though subtraction is still not. I have not yet implemented negation. Have a look @jcrist @flacjacket. Suggestions welcome.

leosartaj · 2015-05-31T18:56:27Z

I have defined multiplication, subtraction. @jcrist @flacjacket

aktech · 2015-05-31T19:36:40Z

sympy/core/tests/test_args.py

@@ -498,7 +497,7 @@ def test_sympy__sets__fancysets__Reals():

 def test_sympy__sets__fancysets__ImageSet():
    from sympy.sets.fancysets import ImageSet
-    from sympy import S, Lambda, Symbol
+    from sympy import S, Symbol


Perhaps symbols are already imported above, you can use that.

I needed to importLambda, which was imported multiple times already. So, I took it out. Well, I will try to cleanup other imports as well. 👍

leosartaj · 2015-06-01T16:26:41Z

I see sequences is a name already taken up and used in python a lot, in a different sense. I think we should change the name to maybe Streams? Suggestion? @flacjacket @jcrist @toolforger

jcrist · 2015-06-01T16:28:54Z

Sequence is fine. It has a different meaning in python, but it also has a specific mathematical meaning - which is what we're implementing here. Streams would be a bad choice IMO. I wouldn't worry about it.

leosartaj · 2015-06-01T16:31:26Z

That's reassuring 👍

jcrist · 2015-06-06T15:16:08Z

I like option 1, with the step being optional. Then users like @certik can enter in a variable with a range like they do for other functionality (i.e. (x, 0, 10)), but can also add a step if needed. I like enclosing the whole variable, range, and step as one object, as they're related. Having the step be its own parameter seems awkward and unintuitive.

That said, I'm not sure why we need a step size - what was your use case for this @leosartaj?

leosartaj · 2015-06-06T15:21:33Z

Well sequences are somewhat like Range. Range does provide an option for step size. So, I also did.

>>> SeqFormula(n**2, (0, oo, 2))
[0, 4, 16, 36, ...]

Also, this type of sequences become easy to represent.

jcrist · 2015-06-06T15:28:34Z

If you feel a step is a good option, I'm fine with that. But consider that your example above is equivalent to:

>>> SeqFormula((2*n)**2, (n, 0, oo))
[0, 4, 16, 36, ...]

certik · 2015-06-06T15:34:31Z

If the step is optional, then I am fine with 1.

Sent from my mobile phone.
On Jun 6, 2015 9:16 AM, "Jim Crist" notifications@github.com wrote:

I like option 1, with the step being optional. Then users like @certik
https://github.com/certik can enter in a variable with a range like
they do for other functionality (i.e. (x, 0, 10)), but can also add a
step if needed. I like enclosing the whole variable, range, and step as one
object, as they're related. Having the step be its own parameter seems
awkward and unintuitive.

That said, I'm not sure why we need a step size - what was your use case
for this @leosartaj https://github.com/leosartaj?

—
Reply to this email directly or view it on GitHub
#9435 (comment).

leosartaj · 2015-06-06T15:40:00Z

Yes, step is optional. So, I will go with 1.

jcrist · 2015-06-06T16:00:50Z

sympy/printing/pretty/pretty.py

+            printset = (dots, s.coeff(stop - 3), s.coeff(stop - 2),
+                s.coeff(stop - 1), s.coeff(stop))
+        elif s.stop is S.Infinity or s.length > 4:
+            it = iter(s)


never used.

jcrist · 2015-06-06T16:08:26Z

I just noticed you never define methods for string printing of your classes. This results in falling back on the default printer (which is fine in many cases). However, as a general rule, the string printed representation should be runnable code (ideally). This is not the case, due to you storing the args in a different format than they were input:

>>> s = SeqFormula((n**2, n), (0, 5))
>>> s
SeqFormula((n**2, n), ([0, 5], 1))
>>> eval(repr(s))
<big error, trimmed traceback>
/home/jimmy/Code/sympy/sympy/core/sympify.py in sympify(a, locals, convert_xor, strict, rational, evaluate)
    270 
    271     if strict:
--> 272         raise SympifyError(a)
    273 
    274     if iterable(a):

SympifyError: SympifyError: [0, 5]

Solutions:

Add methods to the string printer
Store your args identically to their inputs

Either is fine, although the simpler the string printer the better. With the change in api we discussed above, this may work itself out.

toolforger · 2015-06-06T16:51:46Z

TL;DR: I'm -1 on adding step size because the general design rule is to leave out any features that you aren't confident in being used, because the cost (nonzero) will outweigh the benefit (zero). Full argument: Generally speaking, making an API support more facilities always comes with wins and losses. 1) Win: Some use case that one wants to support. 2) Win: Even with no use case, APIs tend to find their application. So one can try it out and see whether people start using it. (You need to instrument the API with usage feedback, else you'll never know whether this win actually exists.) 3) Loss: More effort to write API-compatible code (i.e. subclasses in the case of an OO language). 4) Loss: More effort to check for potential incompatibilities with new extensions. 5) Loss: Higher risk of having to drop an extension because it cannot be made compatible. In this context, we have: 1) Nope. 2) Dubious. An easy workaround exists, if the series becomes too cumbersome people will simply define a symbol and substitute. 3) Given that the Series stuff is new, it's likely that we'll want to write new subclasses. So we have a tangible loss here. 4) Hard to evaluate, since we have no idea how future extensions might look like. We should think more about potential extensions (and maybe use Series in practice), then reevaluate. 5) Essentially the same situation as with (4). With nonexistent or dubious wins, I'd say that it's not worth even a potential loss, much less any losses that we don't know how to evaluate yet.

certik · 2015-06-09T19:27:09Z

@leosartaj please ping me or @jcrist once you update the PR based on the feedback.

leosartaj · 2015-06-10T20:28:45Z

Changes:

Changed the api and made it similar to Sum
Removed step
Removed SeqFunc(redundant with SeqFormula)

@jcrist @certik @flacjacket

jcrist · 2015-06-12T03:16:54Z

Overall this looks very good! One new issue I noticed is that you're inheriting from Expr, not Basic. This allows the following to work, due to how python handles operators:

>>> n = symbols('n')
>>> seq = sequence(n**2)
>>> seq + n
< this errors, but with a poor error message >
>>> n + seq
n + SeqFormula(n**2, (n, 0, oo))

If we're not going to allow sequences to be part of expressions (I don't think we should), then sequences should be rooted in Basic, not Expr. Some arg checking in SeqAdd and SeqMul to detect non sequences would handle the poor error message issue.

Other than that, everything looks good!

jcrist · 2015-06-12T03:19:49Z

sympy/series/sequences.py

+        """
+        return None
+
+    def coeff_mul(self, other):


what is this for? This should be supported by operators, not a special method.

I tried.

>>> sequence(n**2) * 3 sequence(3*n**2) # works

I had trouble

>>> 3 * sequence(n**2) TyperError

Maybe __rmul__ can be used here.

Also, I am not very sure. It may cause some undesired results. Like multiplying sequence with a Tuple.

If this only exists to make __neg__ work (all it's used with, for now), then you can just move this code inside __neg__, and remove the function. I really think operators should only work between two sequences, and that operations with sympy expressions should fail.

I have had it's use in FourierSeries.

Yes only multiplying sequences with sequences should be allowed. I have kept this a separate method. It's works like map. I am having trouble integrating it into __mul__.

Is it okay, if it's kept separate, and * knows how to multiply sequences with sequences only?

Ok, if it's needed elsewhere then it can stay as a method. Perhaps later something prettier can be made.

leosartaj · 2015-06-12T19:17:23Z

Changes:

SeqBase now inherits from Basic
Only sequences can be added, subtracted and multiplied.

@jcrist

leosartaj · 2015-06-14T16:47:10Z

Is this okay? @jcrist

jcrist · 2015-06-14T17:18:21Z

Looks good, merging!

Implementing Sequences

jcrist · 2015-06-14T17:20:18Z

@leosartaj : can you please add something to the release notes under major changes about the new sequences functionality. Thanks for you work!

leosartaj · 2015-06-14T17:21:33Z

Sure, Thanks!

jcrist reviewed May 28, 2015
View reviewed changes

flacjacket reviewed May 28, 2015
View reviewed changes

leosartaj added a commit to leosartaj/sympy that referenced this pull request May 28, 2015

Implemented _validate method in sequence classes

fefdef6

Other small changes: * Got rid of _foo properties (sympy#9435 (comment)) * Other minor changes (Typo and such)

jcrist added the GSoC label May 28, 2015

aktech reviewed May 31, 2015
View reviewed changes

leosartaj force-pushed the sequences branch from cea716c to 4d7568d Compare June 1, 2015 00:00

jcrist reviewed Jun 6, 2015
View reviewed changes

certik added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Jun 9, 2015

leosartaj force-pushed the sequences branch from 35a0cb6 to e3c354b Compare June 10, 2015 20:25

Removed SeqFunc, sequence api similar to Sum

94f46c5

leosartaj force-pushed the sequences branch from e3c354b to 94f46c5 Compare June 11, 2015 15:16

certik added PR: sympy's turn and removed PR: author's turn The PR has been reviewed and the author needs to submit more changes. labels Jun 11, 2015

jcrist reviewed Jun 12, 2015
View reviewed changes

SeqBase inherits from Basic, improved error handling

801f752

jcrist added a commit that referenced this pull request Jun 14, 2015

Merge pull request #9435 from leosartaj/sequences

38d5625

Implementing Sequences

jcrist merged commit 38d5625 into sympy:master Jun 14, 2015

leosartaj mentioned this pull request Jun 14, 2015

Implemented SeriesBase and FourierSeries class #9523

Merged

leosartaj mentioned this pull request Jun 30, 2015

add sphinx docs for sequences #9590

Merged

aktech removed the PR: sympy's turn label Jul 1, 2015

leosartaj deleted the sequences branch July 1, 2015 21:11


		O = Order

		__all__ = ['gruntz', 'limit', 'series', 'O', 'Order', 'Limit', "residue"]

Implementing Sequences #9435

Implementing Sequences #9435

Conversation

leosartaj commented May 27, 2015

TODO

toolforger commented May 27, 2015

leosartaj commented May 27, 2015

toolforger commented May 27, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcrist commented May 28, 2015

leosartaj commented May 28, 2015

Choose a reason for hiding this comment

leosartaj commented May 28, 2015

leosartaj commented May 28, 2015

leosartaj commented May 30, 2015

leosartaj commented May 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leosartaj commented Jun 1, 2015

jcrist commented Jun 1, 2015

leosartaj commented Jun 1, 2015

jcrist commented Jun 6, 2015

leosartaj commented Jun 6, 2015

jcrist commented Jun 6, 2015

certik commented Jun 6, 2015

leosartaj commented Jun 6, 2015

Choose a reason for hiding this comment

jcrist commented Jun 6, 2015

toolforger commented Jun 6, 2015 via email

certik commented Jun 9, 2015

leosartaj commented Jun 10, 2015

jcrist commented Jun 12, 2015

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

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 12, 2015

leosartaj commented Jun 14, 2015

jcrist commented Jun 14, 2015

jcrist commented Jun 14, 2015

leosartaj commented Jun 14, 2015