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:  sage7.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: 
Description (last modified by )
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 withif 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".
Change History (54)
comment:1 Changed 8 years ago by
Branch:  → u/rws/17659 

comment:2 Changed 8 years ago by
Commit:  → a4d2084a82b5f9e2ec3d43472181fccf42a19251 

Status:  new → needs_review 
comment:3 followup: 4 Changed 8 years ago by
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^22)*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 Changed 8 years ago by
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
Description:  modified (diff) 

comment:6 Changed 8 years ago by
Commit:  a4d2084a82b5f9e2ec3d43472181fccf42a19251 → ed003f04b1eefba775e841c0dbfaffdb66d0953a 

Branch pushed to git repo; I updated commit sha1. New commits:
ed003f0  16203: make imports more specific to prevent import loops

comment:7 Changed 8 years ago by
Description:  modified (diff) 

comment:8 followup: 9 Changed 8 years ago by
OK, I thought that f.is_series()
would do some nontrivial 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 Changed 8 years ago by
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 pynac0.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.
comment:10 Changed 8 years ago by
Description:  modified (diff) 

comment:11 Changed 8 years ago by
Commit:  ed003f04b1eefba775e841c0dbfaffdb66d0953a → f97d47d646598385f8f46acea6bc04db4979cc3b 

comment:12 Changed 8 years ago by
Branch:  u/rws/17659 → public/17659 

comment:13 Changed 8 years ago by
Commit:  f97d47d646598385f8f46acea6bc04db4979cc3b → 319860334be59a8470b753b2fdc256bf198503e0 

Branch pushed to git repo; I updated commit sha1. New commits:
3198603  17659: factor out SymbolicSeries from Expression

comment:14 Changed 8 years ago by
Description:  modified (diff) 

comment:15 Changed 8 years ago by
Commit:  319860334be59a8470b753b2fdc256bf198503e0 → a67d0cee6840c90f8dc150734f5a3bd2dc732523 

Branch pushed to git repo; I updated commit sha1. New commits:
a67d0ce  Merge branch 'develop' into t/17659/public/17659

comment:16 Changed 8 years ago by
There is a spurious unreprodcible doctest failure in the 6.6beta2 pachbot run.
comment:17 Changed 8 years ago by
Commit:  a67d0cee6840c90f8dc150734f5a3bd2dc732523 → cb7bbedad5c5afbb815f24c57552c997389e908e 

comment:18 Changed 8 years ago by
Milestone:  sage6.5 → sagepending 

Status:  needs_review → needs_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 sagedevel showed, there is no interest in symbolic series.
comment:19 Changed 7 years ago by
Commit:  cb7bbedad5c5afbb815f24c57552c997389e908e → ce2ebb5f8c20226fc458077df934cbcb8cddacce 

comment:20 Changed 7 years ago by
Milestone:  sagepending → sage6.8 

Status:  needs_work → needs_review 
comment:21 Changed 7 years ago by
Commit:  ce2ebb5f8c20226fc458077df934cbcb8cddacce → 5e2a136fb4fb81f7f17c06b1b037de19159f462d 

Branch pushed to git repo; I updated commit sha1. New commits:
5e2a136  17659: complete doctest coverage

comment:22 Changed 7 years ago by
Milestone:  sage6.8 → sage6.10 

Reviewers:  → Vincent Delecroix 
Status:  needs_review → needs_work 
Hello,
The ticket looks good.
 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.
 You should fix your mind
... about 0, about `1` ...
Either`0`
and`1`
or0
and1
.
 In the example, instead of
float(expr)
could you usenumerical_approx(expr)
?
 You should not put
self
as an argument of a method (you did it intruncate
). Moreover, it is better to avoidself
in the documentation. Always preferthis expression
orthis object
.
 In
coefficients
, the argumentsparse
is not documented in theINPUT
section
 In
coefficients
, you should replaceif sparse is True
byif sparse
.
comment:23 Changed 7 years ago by
Branch:  public/17659 → u/rws/176591 

comment:24 Changed 7 years ago by
Commit:  5e2a136fb4fb81f7f17c06b1b037de19159f462d → fbc9775937aa2078a8923de36f266081b57a94b4 

Status:  needs_work → needs_review 
comment:26 followup: 28 Changed 7 years ago by
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
Commit:  fbc9775937aa2078a8923de36f266081b57a94b4 → b87cc1af8d453e868f78d960d92b22b03ff2ca28 

Branch pushed to git repo; I updated commit sha1. New commits:
b87cc1a  17659: make doctest clearer

comment:28 Changed 7 years ago by
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
Commit:  b87cc1af8d453e868f78d960d92b22b03ff2ca28 → b30cf0a7607bd4847b70950daa1b0798c9b9a9ae 

Branch pushed to git repo; I updated commit sha1. New commits:
b30cf0a  17659: remove superfluous is_series()

comment:31 Changed 7 years ago by
Status:  needs_review → needs_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
Commit:  b30cf0a7607bd4847b70950daa1b0798c9b9a9ae → 7eb36ecea399f47132565939a033e7300208248b 

comment:33 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:34 Changed 7 years ago by
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
Commit:  7eb36ecea399f47132565939a033e7300208248b → d5cc4241d638adb1329adbc737fa29591fc204b6 

Branch pushed to git repo; I updated commit sha1. New commits:
d5cc424  17659: address reviewers comments

comment:36 Changed 7 years ago by
Description:  modified (diff) 

Status:  needs_review → positive_review 
Good for me.
comment:37 Changed 7 years ago by
Status:  positive_review → needs_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:39 Changed 7 years ago by
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
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
I would have found it if it were in the upcoming pynac0.5.2 because I used valgrind extensively there. So it might just disappear with #19312.
comment:42 Changed 7 years ago by
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 Sage6.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
See https://github.com/pynac/pynac/issues/22
This is now fixed in pynac and depends on the upgrade to pynac0.6.1.
comment:44 Changed 7 years ago by
Dependencies:  → #19448 

Status:  needs_work → positive_review 
comment:45 Changed 7 years ago by
Dependencies:  #19448 → #19948 

comment:46 Changed 7 years ago by
Status:  positive_review → needs_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:48 Changed 7 years ago by
Milestone:  sage6.10 → sage7.2 

Status:  needs_work → needs_review 
comment:50 Changed 7 years ago by
Branch:  u/rws/176591 → u/rws/176592 

comment:51 Changed 7 years ago by
Commit:  d5cc4241d638adb1329adbc737fa29591fc204b6 → 1832f4a951c489fa9634beaab7708c443d181c96 

Status:  needs_work → needs_review 
comment:52 Changed 7 years ago by
Status:  needs_review → positive_review 

comment:54 Changed 7 years ago by
Branch:  u/rws/176592 → 1832f4a951c489fa9634beaab7708c443d181c96 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
17659: make symbolic series subclass of Expression