search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.22k stars 424 forks source link

make symbolic series subclass of Expression #17659

Closed rwst closed 8 years ago

rwst commented 9 years ago

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

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

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

Depends on #19948

Component: symbolics

Author: Ralf Stephan

Branch/Commit: 1832f4a

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/17659

rwst commented 9 years ago

Branch: u/rws/17659

rwst commented 9 years ago

Commit: a4d2084

rwst commented 9 years ago

New commits:

a4d208417659: make symbolic series subclass of Expression
nbruin commented 9 years ago
comment:3

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:

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.

rwst commented 9 years ago
comment:4

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.

rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -3,3 +3,4 @@
 * 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".
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from a4d2084 to ed003f0

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

ed003f016203: make imports more specific to prevent import loops
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -4,3 +4,5 @@
 * makes for better documentation via having a `Symbolic Series` ref man page

 In refactoring language this is "replace conditional with polymorphism".
+
+#16302 and #17400 depend on this.
nbruin commented 9 years ago
comment:8

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).

rwst commented 9 years ago
comment:9

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.

rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@

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

-#16302 and #17400 depend on this.
+#16203 and #17400 depend on this.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ed003f0 to f97d47d

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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()
rwst commented 9 years ago

Changed branch from u/rws/17659 to public/17659

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

319860317659: factor out SymbolicSeries from Expression
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from f97d47d to 3198603

rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@

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

-#16203 and #17400 depend on this.
+#16203, #17400, and 17402 depend on this.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 3198603 to a67d0ce

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

a67d0ceMerge branch 'develop' into t/17659/public/17659
rwst commented 9 years ago
comment:16

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from a67d0ce to cb7bbed

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

7183f4dMerge branch 'develop' into t/17659/public/17659
cb7bbed17659: replace PY_NEW
rwst commented 9 years ago
comment:18

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from cb7bbed to ce2ebb5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

5e2a13617659: complete doctest coverage
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ce2ebb5 to 5e2a136

videlec commented 8 years ago

Reviewer: Vincent Delecroix

videlec commented 8 years ago
comment:22

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)?

  2. 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.

  3. In coefficients, the argument sparse is not documented in the INPUT section

  4. In coefficients, you should replace if sparse is True by if sparse.

rwst commented 8 years ago

Changed branch from public/17659 to u/rws/17659-1

rwst commented 8 years ago

Changed commit from 5e2a136 to fbc9775

rwst commented 8 years ago
comment:24

Thanks for looking at this.

New commits:

ab8b1b0Merge branch 'public/17659' of trac.sagemath.org:sage into tmp01
fbc977517659: address reviewer's comments
videlec commented 8 years ago
comment:25

Is the method is_series really usefull?

rwst commented 8 years ago
comment:26

Replying to @videlec:

Is the method is_series really usefull?

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

b87cc1a17659: make doctest clearer
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from fbc9775 to b87cc1a

videlec commented 8 years ago
comment:28

Replying to @rwst:

Replying to @videlec:

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)?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

b30cf0a17659: remove superfluous is_series()
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from b87cc1a to b30cf0a

rwst commented 8 years ago
comment:30

Indeed. Anything else?

videlec commented 8 years ago
comment:31

Replying to @rwst:

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

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

1d9e24517659: add deprecated is_series()
7eb36ec17659: add documentation
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from b30cf0a to 7eb36ec

videlec commented 8 years ago
comment:34

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.)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 7eb36ec to d5cc424

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

d5cc42417659: address reviewers comments
videlec commented 8 years ago
comment:36

Good for me.

videlec commented 8 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,4 @@

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

-#16203, #17400, and 17402 depend on this.
+#16203, #17400, and #17402 depend on this.
vbraun commented 8 years ago
comment:37
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]