Opened 7 years ago
Closed 6 years ago
#17659 closed enhancement (fixed)
make symbolic series subclass of Expression
Reported by:  rws  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 7 years ago by
 Branch set to u/rws/17659
comment:2 Changed 7 years ago by
 Commit set to a4d2084a82b5f9e2ec3d43472181fccf42a19251
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 7 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 in reply to: ↑ 3 Changed 7 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 7 years ago by
 Description modified (diff)
comment:6 Changed 7 years ago by
 Commit changed from a4d2084a82b5f9e2ec3d43472181fccf42a19251 to 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 7 years ago by
 Description modified (diff)
comment:8 followup: ↓ 9 Changed 7 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 in reply to: ↑ 8 Changed 7 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
and fix the worst bugs and that's it. I'd rather improve PowerSeries
but symbolic is what people use.
comment:10 Changed 7 years ago by
 Description modified (diff)
comment:11 Changed 7 years ago by
 Commit changed from ed003f04b1eefba775e841c0dbfaffdb66d0953a to f97d47d646598385f8f46acea6bc04db4979cc3b
comment:12 Changed 7 years ago by
 Branch changed from u/rws/17659 to public/17659
comment:13 Changed 7 years ago by
 Commit changed from f97d47d646598385f8f46acea6bc04db4979cc3b to 319860334be59a8470b753b2fdc256bf198503e0
Branch pushed to git repo; I updated commit sha1. New commits:
3198603  17659: factor out SymbolicSeries from Expression

comment:14 Changed 7 years ago by
 Description modified (diff)
comment:15 Changed 7 years ago by
 Commit changed from 319860334be59a8470b753b2fdc256bf198503e0 to a67d0cee6840c90f8dc150734f5a3bd2dc732523
Branch pushed to git repo; I updated commit sha1. New commits:
a67d0ce  Merge branch 'develop' into t/17659/public/17659

comment:16 Changed 7 years ago by
There is a spurious unreprodcible doctest failure in the 6.6beta2 pachbot run.
comment:17 Changed 7 years ago by
 Commit changed from a67d0cee6840c90f8dc150734f5a3bd2dc732523 to cb7bbedad5c5afbb815f24c57552c997389e908e
comment:18 Changed 7 years ago by
 Milestone changed from sage6.5 to sagepending
 Status changed from needs_review to 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 changed from cb7bbedad5c5afbb815f24c57552c997389e908e to ce2ebb5f8c20226fc458077df934cbcb8cddacce
comment:20 Changed 7 years ago by
 Milestone changed from sagepending to sage6.8
 Status changed from needs_work to needs_review
comment:21 Changed 7 years ago by
 Commit changed from ce2ebb5f8c20226fc458077df934cbcb8cddacce to 5e2a136fb4fb81f7f17c06b1b037de19159f462d
Branch pushed to git repo; I updated commit sha1. New commits:
5e2a136  17659: complete doctest coverage

comment:22 Changed 6 years ago by
 Milestone changed from sage6.8 to sage6.10
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to 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 6 years ago by
 Branch changed from public/17659 to u/rws/176591
comment:24 Changed 6 years ago by
 Commit changed from 5e2a136fb4fb81f7f17c06b1b037de19159f462d to fbc9775937aa2078a8923de36f266081b57a94b4
 Status changed from needs_work to needs_review
comment:25 followup: ↓ 26 Changed 6 years ago by
Is the method is_series
really usefull?
comment:26 in reply to: ↑ 25 ; followup: ↓ 28 Changed 6 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 6 years ago by
 Commit changed from fbc9775937aa2078a8923de36f266081b57a94b4 to b87cc1af8d453e868f78d960d92b22b03ff2ca28
Branch pushed to git repo; I updated commit sha1. New commits:
b87cc1a  17659: make doctest clearer

comment:28 in reply to: ↑ 26 Changed 6 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 6 years ago by
 Commit changed from b87cc1af8d453e868f78d960d92b22b03ff2ca28 to b30cf0a7607bd4847b70950daa1b0798c9b9a9ae
Branch pushed to git repo; I updated commit sha1. New commits:
b30cf0a  17659: remove superfluous is_series()

comment:30 followup: ↓ 31 Changed 6 years ago by
Indeed. Anything else?
comment:31 in reply to: ↑ 30 Changed 6 years ago by
 Status changed from needs_review to 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 6 years ago by
 Commit changed from b30cf0a7607bd4847b70950daa1b0798c9b9a9ae to 7eb36ecea399f47132565939a033e7300208248b
comment:33 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:34 Changed 6 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 6 years ago by
 Commit changed from 7eb36ecea399f47132565939a033e7300208248b to d5cc4241d638adb1329adbc737fa29591fc204b6
Branch pushed to git repo; I updated commit sha1. New commits:
d5cc424  17659: address reviewers comments

comment:36 Changed 6 years ago by
 Description modified (diff)
 Status changed from needs_review to positive_review
Good for me.
comment:37 Changed 6 years ago by
 Status changed from positive_review to 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:38 Changed 6 years ago by
That happens in 6 out of 10 attempts here. Bummer.
comment:39 Changed 6 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 6 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 6 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 6 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 6 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 6 years ago by
 Dependencies set to #19448
 Status changed from needs_work to positive_review
comment:45 Changed 6 years ago by
 Dependencies changed from #19448 to #19948
comment:46 Changed 6 years ago by
 Status changed from positive_review to 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:47 Changed 6 years ago by
Beware the dependency!
comment:48 Changed 6 years ago by
 Milestone changed from sage6.10 to sage7.2
 Status changed from needs_work to needs_review
comment:49 Changed 6 years ago by
 Status changed from needs_review to needs_work
Patchbot also shows failure
comment:50 Changed 6 years ago by
 Branch changed from u/rws/176591 to u/rws/176592
comment:51 Changed 6 years ago by
 Commit changed from d5cc4241d638adb1329adbc737fa29591fc204b6 to 1832f4a951c489fa9634beaab7708c443d181c96
 Status changed from needs_work to needs_review
comment:52 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:53 Changed 6 years ago by
Ralf, did you intend for this to wait to 7.2 or did you mean 7.1?
comment:54 Changed 6 years ago by
 Branch changed from u/rws/176592 to 1832f4a951c489fa9634beaab7708c443d181c96
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
17659: make symbolic series subclass of Expression