Opened 4 years ago

Closed 3 years ago

#17659 closed enhancement (fixed)

make symbolic series subclass of Expression

Reported by: rws 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) Commit: 1832f4a951c489fa9634beaab7708c443d181c96
Dependencies: #19948 Stopgaps:

Description (last modified by vdelecroix)

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 4 years ago by rws

  • Branch set to u/rws/17659

comment:2 Changed 4 years ago by rws

  • Commit set to a4d2084a82b5f9e2ec3d43472181fccf42a19251
  • Status changed from new to needs_review

New commits:

a4d208417659: make symbolic series subclass of Expression

comment:3 follow-up: Changed 4 years ago by 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:

  • 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 4 years ago by rws

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 4 years ago by rws

  • Description modified (diff)

comment:6 Changed 4 years ago by git

  • Commit changed from a4d2084a82b5f9e2ec3d43472181fccf42a19251 to ed003f04b1eefba775e841c0dbfaffdb66d0953a

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

ed003f016203: make imports more specific to prevent import loops

comment:7 Changed 4 years ago by rws

  • Description modified (diff)

comment:8 follow-up: Changed 4 years ago by nbruin

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 4 years ago by rws

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 4 years ago by rws (previous) (diff)

comment:10 Changed 4 years ago by rws

  • Description modified (diff)

comment:11 Changed 4 years ago by git

  • Commit changed from ed003f04b1eefba775e841c0dbfaffdb66d0953a to f97d47d646598385f8f46acea6bc04db4979cc3b

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 4 years ago by rws

  • Branch changed from u/rws/17659 to public/17659

comment:13 Changed 4 years ago by git

  • Commit changed from f97d47d646598385f8f46acea6bc04db4979cc3b to 319860334be59a8470b753b2fdc256bf198503e0

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

319860317659: factor out SymbolicSeries from Expression

comment:14 Changed 4 years ago by rws

  • Description modified (diff)

comment:15 Changed 4 years ago by git

  • Commit changed from 319860334be59a8470b753b2fdc256bf198503e0 to a67d0cee6840c90f8dc150734f5a3bd2dc732523

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

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

comment:16 Changed 4 years ago by rws

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

comment:17 Changed 4 years ago by git

  • Commit changed from a67d0cee6840c90f8dc150734f5a3bd2dc732523 to cb7bbedad5c5afbb815f24c57552c997389e908e

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 4 years ago by rws

  • Milestone changed from sage-6.5 to sage-pending
  • 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 sage-devel showed, there is no interest in symbolic series.

comment:19 Changed 4 years ago by git

  • Commit changed from cb7bbedad5c5afbb815f24c57552c997389e908e to ce2ebb5f8c20226fc458077df934cbcb8cddacce

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 4 years ago by rws

  • Milestone changed from sage-pending to sage-6.8
  • Status changed from needs_work to needs_review

comment:21 Changed 4 years ago by git

  • Commit changed from ce2ebb5f8c20226fc458077df934cbcb8cddacce to 5e2a136fb4fb81f7f17c06b1b037de19159f462d

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

5e2a13617659: complete doctest coverage

comment:22 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.8 to sage-6.10
  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_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 4 years ago by rws

  • Branch changed from public/17659 to u/rws/17659-1

comment:24 Changed 4 years ago by rws

  • Commit changed from 5e2a136fb4fb81f7f17c06b1b037de19159f462d to fbc9775937aa2078a8923de36f266081b57a94b4
  • Status changed from needs_work to needs_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 follow-up: Changed 4 years ago by vdelecroix

Is the method is_series really usefull?

comment:26 in reply to: ↑ 25 ; follow-up: Changed 4 years ago by 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?

comment:27 Changed 4 years ago by git

  • Commit changed from fbc9775937aa2078a8923de36f266081b57a94b4 to b87cc1af8d453e868f78d960d92b22b03ff2ca28

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

b87cc1a17659: make doctest clearer

comment:28 in reply to: ↑ 26 Changed 4 years ago by vdelecroix

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 4 years ago by git

  • Commit changed from b87cc1af8d453e868f78d960d92b22b03ff2ca28 to b30cf0a7607bd4847b70950daa1b0798c9b9a9ae

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

b30cf0a17659: remove superfluous is_series()

comment:30 follow-up: Changed 4 years ago by rws

Indeed. Anything else?

comment:31 in reply to: ↑ 30 Changed 4 years ago by vdelecroix

  • 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 4 years ago by git

  • Commit changed from b30cf0a7607bd4847b70950daa1b0798c9b9a9ae to 7eb36ecea399f47132565939a033e7300208248b

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

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

comment:33 Changed 4 years ago by rws

  • Status changed from needs_work to needs_review

comment:34 Changed 4 years ago by vdelecroix

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 4 years ago by git

  • Commit changed from 7eb36ecea399f47132565939a033e7300208248b to d5cc4241d638adb1329adbc737fa29591fc204b6

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

d5cc42417659: address reviewers comments

comment:36 Changed 4 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Good for me.

comment:37 Changed 4 years ago by vbraun

  • 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 4 years ago by rws

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

comment:39 Changed 4 years ago by rws

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 4 years ago by vbraun

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 4 years ago by rws

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 3 years ago by rws

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 3 years ago by rws

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 3 years ago by rws

  • Dependencies set to #19448
  • Status changed from needs_work to positive_review

comment:45 Changed 3 years ago by rws

  • Dependencies changed from #19448 to #19948

comment:46 Changed 3 years ago by vbraun

  • 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 3 years ago by rws

Beware the dependency!

comment:48 Changed 3 years ago by rws

  • Milestone changed from sage-6.10 to sage-7.2
  • Status changed from needs_work to needs_review

comment:49 Changed 3 years ago by vbraun

  • Status changed from needs_review to needs_work

Patchbot also shows failure

comment:50 Changed 3 years ago by rws

  • Branch changed from u/rws/17659-1 to u/rws/17659-2

comment:51 Changed 3 years ago by rws

  • Commit changed from d5cc4241d638adb1329adbc737fa29591fc204b6 to 1832f4a951c489fa9634beaab7708c443d181c96
  • Status changed from needs_work to needs_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 3 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:53 Changed 3 years ago by tscrim

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

comment:54 Changed 3 years ago by vbraun

  • Branch changed from u/rws/17659-2 to 1832f4a951c489fa9634beaab7708c443d181c96
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.