Opened 8 years ago

Closed 7 years ago

#17659 closed enhancement (fixed)

make symbolic series subclass of Expression

Reported by: Ralf Stephan Owned by:
Priority: major Milestone: sage-7.2
Component: symbolics Keywords:
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 1832f4a (Commits, GitHub, GitLab) Commit: 1832f4a951c489fa9634beaab7708c443d181c96
Dependencies: #19948 Stopgaps:

Status badges

Description (last modified by Vincent Delecroix)

Making Expression.series create SymbolicSeries (a subclass of Expression) and moving the series code into a separate file series.pyx

  • reflects the fact that expressions and series don't commute
  • allows upcoming fixes that don't clutter Expression methods with if ex.is_a_series():
  • makes for better documentation via having a Symbolic Series ref man page

In refactoring language this is "replace conditional with polymorphism".

#16203, #17400, and #17402 depend on this.

Change History (54)

comment:1 Changed 8 years ago by Ralf Stephan

Branch: u/rws/17659

comment:2 Changed 8 years ago by Ralf Stephan

Commit: a4d2084a82b5f9e2ec3d43472181fccf42a19251
Status: newneeds_review

New commits:

a4d208417659: make symbolic series subclass of Expression

comment:3 Changed 8 years ago by Nils Bruin

I have nothing invested in how symbolics and series interact, but I see some immediate problems that would arise from subclassing Expression for a Series type:

  • when does a SymbolicSeries expression get created? The problem is that the SymbolicRing by itself is a rather untyped environment, so how do you know that an object needs to be made in this specialized class
  • How do SymbolicSeries in different variables interact, e.g.:
    F = (1/(x+1))*y+sin(x)*y^2+O(y^3)
    G = (y/(y^2-2)*x + cos(y)*x^2 +O(x^3)
    F+G
    

Is the result a series? If so, in which variable(s)? One solution would be a SymbolicSeriesRing? as a parent, which declares in what variables the series are. I suspect this would be nearly useless for whatever problem the current design tries to solve, though.

comment:4 in reply to:  3 Changed 8 years ago by Ralf Stephan

Nils, this is not about creating a ring but simple refactoring. No behaviour change.

http://www.refactoring.com/catalog/replaceConditionalWithPolymorphism.html

Replying to nbruin:

I have nothing invested in how symbolics and series interact, but I see some immediate problems that would arise from subclassing Expression for a Series type:

These problems already exist, so nothing is new.

comment:5 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:6 Changed 8 years ago by git

Commit: a4d2084a82b5f9e2ec3d43472181fccf42a19251ed003f04b1eefba775e841c0dbfaffdb66d0953a

Branch pushed to git repo; I updated commit sha1. New commits:

ed003f016203: make imports more specific to prevent import loops

comment:7 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:8 Changed 8 years ago by Nils Bruin

OK, I thought that f.is_series() would do some non-trivial logic to see if f can be used as a series, but it's just exposing a flag that is held internally somewhere. You could indeed expose that information in the type instead, but with ducktyping, subtyping and sage's parent infrastructure doing so might not be as convenient as it might be in systems that are really governed by explicit types. You'd need to think if this refactoring actually will help for further development.

Indeed, being a "series" seems a rather fickle property. It doesn't seem to be preserved under anything:

sage: var('x,y');
sage: f=cos(y).series(y,10)
sage: g=sin(x).series(x,10)
sage: var('x,y');
sage: f=cos(y).series(y,10)
sage: g=sin(x).series(x,10)
sage: f.is_series()
True
sage: (f+f).is_series()
False
sage: (-f).is_series()
False
sage: (f+g).series(x,10)
Order(1)

The last result probably follows from interpreting the Order(x^10) term in g as an order term in y, and hence equivalent to Order(y^0).

comment:9 in reply to:  8 Changed 8 years ago by Ralf Stephan

Replying to nbruin:

You'd need to think if this refactoring actually will help for further development.

It isolates the code and the documentation.

Indeed, being a "series" seems a rather fickle property. It doesn't seem to be preserved under anything:

There are functions in pynac-0.3.2/ginac/pseries.cpp to compare, add, multiply (also with constant), power (to constant). My plan is to only provide conversion to and from PowerSeries (#10846, #16203, #17402) and fix the worst bugs (#17400, #16213) and that's it. I'd rather improve PowerSeries but symbolic is what people use.

Last edited 8 years ago by Ralf Stephan (previous) (diff)

comment:10 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:11 Changed 8 years ago by git

Commit: ed003f04b1eefba775e841c0dbfaffdb66d0953af97d47d646598385f8f46acea6bc04db4979cc3b

Branch pushed to git repo; I updated commit sha1. New commits:

02885afMerge branch 'develop' into t/17659/17659
f97d47d17659: add SymbolicSeries.default_variable() and coefficients()

comment:12 Changed 8 years ago by Ralf Stephan

Branch: u/rws/17659public/17659

comment:13 Changed 8 years ago by git

Commit: f97d47d646598385f8f46acea6bc04db4979cc3b319860334be59a8470b753b2fdc256bf198503e0

Branch pushed to git repo; I updated commit sha1. New commits:

319860317659: factor out SymbolicSeries from Expression

comment:14 Changed 8 years ago by Ralf Stephan

Description: modified (diff)

comment:15 Changed 8 years ago by git

Commit: 319860334be59a8470b753b2fdc256bf198503e0a67d0cee6840c90f8dc150734f5a3bd2dc732523

Branch pushed to git repo; I updated commit sha1. New commits:

a67d0ceMerge branch 'develop' into t/17659/public/17659

comment:16 Changed 8 years ago by Ralf Stephan

There is a spurious unreprodcible doctest failure in the 6.6beta2 pachbot run.

comment:17 Changed 8 years ago by git

Commit: a67d0cee6840c90f8dc150734f5a3bd2dc732523cb7bbedad5c5afbb815f24c57552c997389e908e

Branch pushed to git repo; I updated commit sha1. New commits:

7183f4dMerge branch 'develop' into t/17659/public/17659
cb7bbed17659: replace PY_NEW

comment:18 Changed 8 years ago by Ralf Stephan

Milestone: sage-6.5sage-pending
Status: needs_reviewneeds_work

This now uncovers a regression of #12255 which was not really fixed.

File "src/sage/symbolic/expression.pyx", line 5279, in sage.symbolic.expression.Expression.coefficients
Failed example:
    f.coefficients(g)
Exception raised:
    ValueError: The name "g(t)" is not a valid Python identifier.

I will now let this ticket (and with it its dependencies) lie in limbo because, as a posting on sage-devel showed, there is no interest in symbolic series.

comment:19 Changed 7 years ago by git

Commit: cb7bbedad5c5afbb815f24c57552c997389e908ece2ebb5f8c20226fc458077df934cbcb8cddacce

Branch pushed to git repo; I updated commit sha1. New commits:

1bdef21Merge branch 'develop' into t/17659/public/17659
ce2ebb517659: remove nonsensical code to fix doctest failure

comment:20 Changed 7 years ago by Ralf Stephan

Milestone: sage-pendingsage-6.8
Status: needs_workneeds_review

comment:21 Changed 7 years ago by git

Commit: ce2ebb5f8c20226fc458077df934cbcb8cddacce5e2a136fb4fb81f7f17c06b1b037de19159f462d

Branch pushed to git repo; I updated commit sha1. New commits:

5e2a13617659: complete doctest coverage

comment:22 Changed 7 years ago by Vincent Delecroix

Milestone: sage-6.8sage-6.10
Reviewers: Vincent Delecroix
Status: needs_reviewneeds_work

Hello,

The ticket looks good.

  1. Why do you need all these include
    include "sage/ext/interrupt.pxi"
    include "sage/ext/stdsage.pxi"
    include "sage/ext/cdefs.pxi"
    include "sage/ext/python.pxi"
    
    As far as I saw you are using none of them.
  1. You should fix your mind
    ... about 0, about `1` ...
    
    Either `0` and `1` or 0 and 1.
  1. In the example, instead of float(expr) could you use numerical_approx(expr)?
  1. You should not put self as an argument of a method (you did it in truncate). Moreover, it is better to avoid self in the documentation. Always prefer this expression or this object.
  1. In coefficients, the argument sparse is not documented in the INPUT section
  1. In coefficients, you should replace if sparse is True by if sparse.

comment:23 Changed 7 years ago by Ralf Stephan

Branch: public/17659u/rws/17659-1

comment:24 Changed 7 years ago by Ralf Stephan

Commit: 5e2a136fb4fb81f7f17c06b1b037de19159f462dfbc9775937aa2078a8923de36f266081b57a94b4
Status: needs_workneeds_review

Thanks for looking at this.


New commits:

ab8b1b0Merge branch 'public/17659' of trac.sagemath.org:sage into tmp01
fbc977517659: address reviewer's comments

comment:25 Changed 7 years ago by Vincent Delecroix

Is the method is_series really usefull?

comment:26 in reply to:  25 ; Changed 7 years ago by Ralf Stephan

Replying to vdelecroix:

Is the method is_series really usefull?

Series are still expressions. How would you know a symbol holding a specific expression holds a series?

comment:27 Changed 7 years ago by git

Commit: fbc9775937aa2078a8923de36f266081b57a94b4b87cc1af8d453e868f78d960d92b22b03ff2ca28

Branch pushed to git repo; I updated commit sha1. New commits:

b87cc1a17659: make doctest clearer

comment:28 in reply to:  26 Changed 7 years ago by Vincent Delecroix

Replying to rws:

Replying to vdelecroix:

Is the method is_series really usefull?

Series are still expressions. How would you know a symbol holding a specific expression holds a series?

instance(my_object, SymbolicSeries)?

comment:29 Changed 7 years ago by git

Commit: b87cc1af8d453e868f78d960d92b22b03ff2ca28b30cf0a7607bd4847b70950daa1b0798c9b9a9ae

Branch pushed to git repo; I updated commit sha1. New commits:

b30cf0a17659: remove superfluous is_series()

comment:30 Changed 7 years ago by Ralf Stephan

Indeed. Anything else?

comment:31 in reply to:  30 Changed 7 years ago by Vincent Delecroix

Status: needs_reviewneeds_work

Replying to rws:

Indeed. Anything else?

Yes: you are not allowed to remove a public function without (one year) deprecation with an error message that indicates the new procedure: http://doc.sagemath.org/html/en/developer/coding_in_python.html#deprecation

comment:32 Changed 7 years ago by git

Commit: b30cf0a7607bd4847b70950daa1b0798c9b9a9ae7eb36ecea399f47132565939a033e7300208248b

Branch pushed to git repo; I updated commit sha1. New commits:

1d9e24517659: add deprecated is_series()
7eb36ec17659: add documentation

comment:33 Changed 7 years ago by Ralf Stephan

Status: needs_workneeds_review

comment:34 Changed 7 years ago by Vincent Delecroix

Isn't it dangerous to expose globally SymbolicSeries? If people don't know about classes they do not want to see it. And if they do, they know how to import modules. You can just be explicit in the deprecation message

isinstance(my_expr, sage.symbolic.series.SymbolicSeries)

(I would also hide Expression from the global namespace but this does not concern this ticket.)

comment:35 Changed 7 years ago by git

Commit: 7eb36ecea399f47132565939a033e7300208248bd5cc4241d638adb1329adbc737fa29591fc204b6

Branch pushed to git repo; I updated commit sha1. New commits:

d5cc42417659: address reviewers comments

comment:36 Changed 7 years ago by Vincent Delecroix

Description: modified (diff)
Status: needs_reviewpositive_review

Good for me.

comment:37 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_work
sage -t --long src/sage/symbolic/series.pyx
**********************************************************************
File "src/sage/symbolic/series.pyx", line 71, in sage.symbolic.series
Failed example:
    x*ex1
Expected:
    (1*x + (-1/6)*x^3 + Order(x^4))*x
Got:
    x*(1*x + (-1/6)*x^3 + Order(x^4))
**********************************************************************
1 item had failures:
   1 of  23 in sage.symbolic.series
    [42 tests, 1 failure, 0.04 s]

comment:38 Changed 7 years ago by Ralf Stephan

That happens in 6 out of 10 attempts here. Bummer.

comment:39 Changed 7 years ago by Ralf Stephan

For the record, only when doctesting and in different processes does it fail often enough to be noticed (regardless of if testing in parallel or not). When not leaving the Sage process the results seem always the same. One possbility would be therefore an uninitialized variable in Pynac affecting the result depending on what's in the memory at Sage start.

comment:40 Changed 7 years ago by Volker Braun

Might also depend on Python memory locations (are the underlying ginac objects cached or not?). If its an uninitialized pynac variable then valgrind should find it.

comment:41 Changed 7 years ago by Ralf Stephan

I would have found it if it were in the upcoming pynac-0.5.2 because I used valgrind extensively there. So it might just disappear with #19312.

comment:42 Changed 7 years ago by Ralf Stephan

Okay, for some reason printing order here is according to hash value, and hash values of symbols are not preserved over different processes. Putting sage: hash(x) in an arbitrary doctest and testing multiple times shows this clearly, also in Sage-6.6. It's just that such series manipulations produced an error until recently, so the differences over time couldn't be observed.

comment:43 Changed 7 years ago by Ralf Stephan

See https://github.com/pynac/pynac/issues/22

This is now fixed in pynac and depends on the upgrade to pynac-0.6.1.

comment:44 Changed 7 years ago by Ralf Stephan

Dependencies: #19448
Status: needs_workpositive_review

comment:45 Changed 7 years ago by Ralf Stephan

Dependencies: #19448#19948

comment:46 Changed 7 years ago by Volker Braun

Status: positive_reviewneeds_work
sage -t --long src/sage/symbolic/series.pyx
**********************************************************************
File "src/sage/symbolic/series.pyx", line 71, in sage.symbolic.series
Failed example:
    x*ex1
Expected:
    (1*x + (-1/6)*x^3 + Order(x^4))*x
Got:
    x*(1*x + (-1/6)*x^3 + Order(x^4))
**********************************************************************
1 item had failures:
   1 of  23 in sage.symbolic.series
    [42 tests, 1 failure, 0.04 s]

comment:47 Changed 7 years ago by Ralf Stephan

Beware the dependency!

comment:48 Changed 7 years ago by Ralf Stephan

Milestone: sage-6.10sage-7.2
Status: needs_workneeds_review

comment:49 Changed 7 years ago by Volker Braun

Status: needs_reviewneeds_work

Patchbot also shows failure

comment:50 Changed 7 years ago by Ralf Stephan

Branch: u/rws/17659-1u/rws/17659-2

comment:51 Changed 7 years ago by Ralf Stephan

Commit: d5cc4241d638adb1329adbc737fa29591fc204b61832f4a951c489fa9634beaab7708c443d181c96
Status: needs_workneeds_review

Yes, a remainder from the time before #19948.


New commits:

c3be6a1Merge branch 'u/rws/17659-1' of trac.sagemath.org:sage into tmp04
1832f4a17659: fix doctest

comment:52 Changed 7 years ago by Volker Braun

Status: needs_reviewpositive_review

comment:53 Changed 7 years ago by Travis Scrimshaw

Ralf, did you intend for this to wait to 7.2 or did you mean 7.1?

comment:54 Changed 7 years ago by Volker Braun

Branch: u/rws/17659-21832f4a951c489fa9634beaab7708c443d181c96
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.