#22801 closed enhancement (fixed)

SymPy as optional symbolic method for manifolds

Reported by: mmancini Owned by:
Priority: major Milestone: sage-8.1
Component: geometry Keywords:
Cc: egourgoulhon Merged in:
Authors: Marco Mancini Reviewers: Eric Gourgoulhon, Ralf Stephan, Richard Lozes
Report Upstream: N/A Work issues:
Branch: 87877f0 (Commits) Commit: 87877f05a3908e13507afdc281289853a7e1e946
Dependencies: #22802 #24006 #20204 #24232 Stopgaps:

Description (last modified by mmancini)

The symbolic method used for manifolds and differentiable manifolds is "symbolic", the default in SageMath. In this ticket we introduce the use of SymPy? as an option when the Chart in the manifold is created.

At this point there is a dependence on Sympy ticket: "sympy to sage conversion : Abstract function." https://github.com/sympy/sympy/pull/12826

Attachments (3)

22801_4threads.failure (5.3 KB) - added by rllozes 14 months ago.
22801_proc _gt_1.sws (3.2 KB) - added by rllozes 14 months ago.
Showing failure with computations using sympy and parallelism (proc>1)
22801_proc_eq_1.sws (1.8 KB) - added by rllozes 13 months ago.

Download all attachments as: .zip

Change History (106)

comment:1 Changed 20 months ago by mmancini

  • Dependencies set to #22802

comment:2 Changed 20 months ago by mmancini

  • Branch set to 22801-manifold-sympy

comment:3 Changed 20 months ago by mmancini

  • Branch changed from 22801-manifold-sympy to public/22801-manifold-sympy
  • Commit set to 3da57124a6e7cd88ba7928c0a5bdf67603f321c6

New commits:

b494a4fAdded sympy symbolic method for manifolds.
3da5712merged with develop

comment:4 Changed 20 months ago by egourgoulhon

  • Cc egourgoulhon added; Eric Gourgoulhon removed

comment:5 Changed 19 months ago by rws

  • Component changed from symbolics to geometry

comment:6 Changed 19 months ago by git

  • Commit changed from 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 to 3be45473000c9141d38710f71ea99373b3c747c0

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

3be4547created the class ChartFunction which will replace the CoordFunction. also the singleton to manage symbolic method was created

comment:7 Changed 19 months ago by git

  • Commit changed from 3be45473000c9141d38710f71ea99373b3c747c0 to 01bc6644d4d2647c465920591ff496108f9bcf67

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

01bc664Merge branch 'public/22801-manifold-sympy' of git://trac.sagemath.org/sage into public/22801-manifold-sympy

comment:8 Changed 18 months ago by git

  • Commit changed from 01bc6644d4d2647c465920591ff496108f9bcf67 to b07e5fb080271ce765a1f5d27d6b60089e091989

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

b07e5fbChartFunction pass almost all tests

comment:9 Changed 18 months ago by git

  • Commit changed from b07e5fb080271ce765a1f5d27d6b60089e091989 to 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4

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

9a1a1f2added all test for sympy in chart_func
4998a56Changes in chart_func are propagated on all manifolds files.
1e2dc01Propagated changes in manifolds/differentiable and added many tests.

comment:10 Changed 18 months ago by mmancini

  • Description modified (diff)

comment:11 Changed 17 months ago by git

  • Commit changed from 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4 to 7fcf258a2ca94c6138a721bd816ddb0a98e11cb2

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

7fcf258passing all test if src/sage/symbolic/expression_conversions.py and changed as in #22802

comment:12 Changed 15 months ago by git

  • Commit changed from 7fcf258a2ca94c6138a721bd816ddb0a98e11cb2 to 66f73dfedce548c8bc61b14ba5f671d5111ac480

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

068d495Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function
078532fdocstring modifications
67f7f46integrated modifications to composition. But an error remains in french_book.recequadiff tests
1b5227dError in french_book.recequadiff tests corrected
018d753Merge branch 'develop' into t/23496/public/23496_patch_sympy_abstact_function
94af74b#23496 : (temp) revert order in rsolve recequadiff.tex test
d76caea#23496 : add message for change in doctest
bc21300#23496 : fix a _sympy_init_ doctest
0be1d90Merge branch 'public/23496_patch_sympy_abstract_function' into 22802_sympy_abstract_function
66f73dfMerge branch 'public/22802_sympy_abstract_function' of git://trac.sagemath.org/sage into public/22801-manifold-sympy

comment:13 Changed 15 months ago by git

  • Commit changed from 66f73dfedce548c8bc61b14ba5f671d5111ac480 to fdb396baa4a2c5027c7a890b22de675949512669

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

fdb396bMinor changes in chart_func

comment:14 Changed 15 months ago by git

  • Commit changed from fdb396baa4a2c5027c7a890b22de675949512669 to 8f20b54684118bf5c274c540893d397e45d3171f

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

8f20b54Merge branch 'develop' into public/22801-manifold-sympy

comment:15 Changed 15 months ago by mmancini

  • Reviewers set to Eric Gourgoulhon
  • Status changed from new to needs_review

comment:16 Changed 15 months ago by git

  • Commit changed from 8f20b54684118bf5c274c540893d397e45d3171f to 102a3e124d9c5e6a766cae6352ab1956191dc2c9

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

102a3e1error in docstring

comment:17 Changed 15 months ago by tscrim

  • Status changed from needs_review to needs_work

There are a few things that need fixing.

  • Not all methods have docstrings/tests
  • There are a number of tests where the line before is informational, but it needs a blankline, e.g.:
    @@ -1641,6 +1641,19 @@ class TopologicalManifold(ManifoldSubset):
                 F: U --> R
                    (x, y, z) |--> cos(y)*sin(x) + z
     
    +
    +        Same tests with ``sympy``::
    +            sage: c_xyz.set_calculus_method('sympy')
    +            sage: f = U.scalar_field(sin(x)*cos(y) + z, name='F'); f
    +            Scalar field F on the Open subset U of the 3-dimensional topological manifold M
    +            sage: f.display()
    +            F: U --> R
    +               (x, y, z) |--> cos(y)*sin(x) + z
    +
    +            sage: type(f.coord_function(c_xyz).expr())
    +               <class 'sympy.core.add.Add'>
    +
    +
    
  • IMO, you are too liberal with your blanklines spacing things out that makes the code a little uneven to read.

comment:18 Changed 15 months ago by git

  • Commit changed from 102a3e124d9c5e6a766cae6352ab1956191dc2c9 to 4800d97f9dca68300c1799541b4b208a5b43d023

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

4800d97Added better (I hope) docstring

comment:19 follow-up: Changed 15 months ago by mmancini

  • Status changed from needs_work to needs_review

I hope that now the docstring is better (and less liberal)

comment:20 in reply to: ↑ 19 Changed 15 months ago by egourgoulhon

Replying to mmancini:

I hope that now the docstring is better (and less liberal)

Thanks a lot for these improvements! I'll have a look at the code next week. Just a quick comment: as revealed by the patchbot, there are still some coverage issue, as well as a problem with sage/manifolds/chart_func.py when building the documentation. There are also some doctest failures.

comment:21 Changed 15 months ago by git

  • Commit changed from 4800d97f9dca68300c1799541b4b208a5b43d023 to ae47c5d7311287403f0ba44e943cc322c33da315

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

ae47c5dAgain changes in docstring

comment:22 Changed 15 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

According to the patchbots, there are still failing doctests in tensorfield.py and the doc does not build.

comment:23 Changed 15 months ago by git

  • Commit changed from ae47c5d7311287403f0ba44e943cc322c33da315 to 611f5de331fef531e0ac7effd6a88fa70f743103

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

611f5dedocstring: again problems with sympy

comment:24 Changed 15 months ago by mmancini

  • Status changed from needs_work to needs_review

comment:25 Changed 15 months ago by egourgoulhon

  • Status changed from needs_review to needs_work

The last commit fixes the doc build, but not the doctest failures in tensorfield.py.

comment:26 Changed 14 months ago by git

  • Commit changed from 611f5de331fef531e0ac7effd6a88fa70f743103 to dd82a154e88edc0be9246337c7146e7088ab8b77

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

dd82a15chart_func : changing again display depending on calculus method

comment:27 Changed 14 months ago by git

  • Commit changed from dd82a154e88edc0be9246337c7146e7088ab8b77 to c1d9e4fec66987ea82a7b22d298839979ed15ac0

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

c1d9e4fchart_func : changing again display depending on calculus method. All test passed

comment:28 Changed 14 months ago by mmancini

  • Status changed from needs_work to needs_review

Display for ChartFunction? has been changed: now the result is different for Sympy and SR. All test results have been modified.

comment:29 follow-up: Changed 14 months ago by git

  • Commit changed from c1d9e4fec66987ea82a7b22d298839979ed15ac0 to fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf

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

fb0fb7eSymPy on manifolds: first reviewer tweaks (more to come)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 14 months ago by mmancini

Replying to git:

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

fb0fb7eSymPy on manifolds: first reviewer tweaks (more to come)

I see in the difference the following one:

@@ -231,7 +225,7 @@ class DiffForm(TensorField):
         sage: a.display(eU)
         a = x*(y**2 + 2) dx/\dy
         sage: a.display(eV)
-        a = (-u**3/16 + u**2*v/16 + u*v**2/16 - u/2 - v**3/16 - v/2) du/\dv
+        a = (-u**3/16 + u*v**2/16 - u/2 - v**3/16 + v*(u**2 - 8)/16) du/\dv

I found is surprising : do you know where this difference comes from?

comment:31 in reply to: ↑ 30 Changed 14 months ago by egourgoulhon

Replying to mmancini:

I see in the difference the following one:

@@ -231,7 +225,7 @@ class DiffForm(TensorField):
         sage: a.display(eU)
         a = x*(y**2 + 2) dx/\dy
         sage: a.display(eV)
-        a = (-u**3/16 + u**2*v/16 + u*v**2/16 - u/2 - v**3/16 - v/2) du/\dv
+        a = (-u**3/16 + u*v**2/16 - u/2 - v**3/16 + v*(u**2 - 8)/16) du/\dv

I found is surprising : do you know where this difference comes from?

It is due to a different setting: in the previous version, the transition map xy_to_uv was redefined, thereby causing a different handling of computations between SR and sympy. Note that the two expressions of a are mathematically equivalent, which is satisfactory.

comment:32 Changed 14 months ago by git

  • Commit changed from fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf to f985a2f3fed328b8b008f53afb080691d32c64b7

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

f985a2fSymPy on manifolds: second reviewer tweaks (a slight more to come)

comment:33 follow-up: Changed 14 months ago by egourgoulhon

The above commit contains some polishing of the code (mostly layout) and improvements in the documentation. In particular, it restores the section the rubric Differences between chart fucntions and callable symbolic expressions of the previous class CoordFunctionSymb.

Two questions:

  • Why is there an argument expression in method ChartFunction._simplify? I mean, if expression is not self.expr(method=calc_method), why not calling directly the simplify method of CalculusMethod on it?
  • In the docstring of ChartFunctionRing._coerce_map_from_, there is a TODO section with There is a problem with coercion with rationals. What is that?

comment:34 in reply to: ↑ 33 Changed 14 months ago by mmancini

Replying to egourgoulhon:

The above commit contains some polishing of the code (mostly layout) and improvements in the documentation. In particular, it restores the section the rubric Differences between chart fucntions and callable symbolic expressions of the previous class CoordFunctionSymb.

Thanks

Two questions:

  • Why is there an argument expression in method ChartFunction._simplify? I mean, if expression is not self.expr(method=calc_method), why not calling directly the simplify method of CalculusMethod on it?

At a particular moment during development I introduced it but I don't remember its functionality. As it is used only internally to ChartFunction I will remove this argument.

  • In the docstring of ChartFunctionRing._coerce_map_from_, there is a TODO section with There is a problem with coercion with rationals. What is that?

Look at this example:

M = Manifold(2, 'M', structure='topological')
X.<x,y> = M.chart()
f = X.function(x^2+3*y+1)
g = f+1/2

TypeError                                 Traceback (most recent call last)
<ipython-input-5-83503284e242> in <module>()
----> 1 g=f+Integer(1)/Integer(2)

/home/mmancini/Sage-last/sage/src/sage/structure/element.pyx in sage.structure.element.Element.__add__ (build/cythonized/sage/structure/element.c:10823)()
   1240         # Left and right are Sage elements => use coercion model
   1241         if BOTH_ARE_ELEMENT(cl):
-> 1242             return coercion_model.bin_op(left, right, add)
   1243 
   1244         try:

...
...
...

comment:35 Changed 14 months ago by git

  • Commit changed from f985a2f3fed328b8b008f53afb080691d32c64b7 to c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f

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

c8b3bdcChartFunction: modification of _symplify

comment:36 Changed 14 months ago by git

  • Commit changed from c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f to e1052fab39827595b670bb4d4b5651102781fb3b

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

e1052faUse of SymPy subs when relevant in ChartFunction.__call__

comment:37 Changed 14 months ago by egourgoulhon

The above commit

  • makes _simplify a mere shortcut to CalculusMethod.simplify in ChartFunction
  • introduces the data member ._chart in class ChartFunction
  • uses SymPy substitutions (instead of forcing a conversion to SR) in ChartFunction.__call__ when dealing with SymPy expressions
  • makes sure that simplify_chain_real_sympy returns a SymPy object
  • fixes the issue of coercion of QQ to ChartFunctionRing

comment:38 Changed 14 months ago by git

  • Commit changed from e1052fab39827595b670bb4d4b5651102781fb3b to 124c582421b49afc38cdd565d0057031c347b044

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

124c582Improve SymPy documentation in sage/manifolds/scalarfield.py

comment:39 Changed 14 months ago by git

  • Commit changed from 124c582421b49afc38cdd565d0057031c347b044 to eea0070a852b76cee592047307bb273172e2dc71

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

eea0070Change in SymPy conversion function of CalculusMethod + use of SymPy variable in ChartFunction.diff when relevant

comment:40 Changed 14 months ago by git

  • Commit changed from eea0070a852b76cee592047307bb273172e2dc71 to 07bef4df2312f4413a05e2808d3ddf33b80e1195

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

07bef4dChanging docstring for test results in Sympy

comment:41 Changed 14 months ago by git

  • Commit changed from 07bef4df2312f4413a05e2808d3ddf33b80e1195 to e5ea40e52531f2b1d063df2000b786ebc005dde3

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

e5ea40e- Small error in inverse calculation resolved

comment:42 Changed 14 months ago by git

  • Commit changed from e5ea40e52531f2b1d063df2000b786ebc005dde3 to 80f67852937d2a2ebffaaf7d0b8318dd9e5847b9

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

946295423990: Convert symbolic relations to SymPy
07f474423990: handle unequality
0ed659f23990: do not evaluate
5f023ea23990: convert relations from SymPy to Sage, with test
0f596bb23990: fix patch
479e20623990: sympy patchlevel bump
3e362ee24006: SymPy --> Sage conversion completely inside Sage
8dd5f8824006: add missing file, fixes
e0310ddMerge branch 'u/rws/24006' of git://trac.sagemath.org/sage into public/22801-manifold-sympy
80f6785Resolved the problem of simplification in S2 Chart.set_inverse function

comment:43 Changed 14 months ago by mmancini

  • Dependencies changed from #22802 to #22802 #24006

comment:44 Changed 14 months ago by rws

Better base everything on #20204 as it contains all SymPy Interface improvements, ie abstract functions.

comment:45 Changed 14 months ago by git

  • Commit changed from 80f67852937d2a2ebffaaf7d0b8318dd9e5847b9 to 952970d2b293ad801ced530645a77cea7dc6d9bc

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

a68d57f23923: Interface cases function with SymPy's piecewise
b80d98c23923: fix typo
2afa0ee22566: interface SymPy's ceiling()
9494f6c22566: add test
8215d37Merge branch 'develop' into tmp05
7899ea920204: convert SymPy abstract functions
b261ec323923: fix doctest
5010acfMerge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204
952970dMerge branch 'u/rws/20204' of git://trac.sagemath.org/sage into public/22801-manifold-sympy

comment:46 Changed 14 months ago by mmancini

  • Dependencies changed from #22802 #24006 to #22802 #24006 #20204

comment:47 Changed 14 months ago by git

  • Commit changed from 952970d2b293ad801ced530645a77cea7dc6d9bc to 1ff225cce1ffa1f84664bc58002099b883a2fef5

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

1ff225cAdded Subs._sage_. Now differentiation in manifold work. A problem in dispaly in diff_form.py appeared

comment:48 Changed 14 months ago by mmancini

  • Status changed from needs_review to needs_work

comment:49 Changed 14 months ago by git

  • Commit changed from 1ff225cce1ffa1f84664bc58002099b883a2fef5 to ff540a3994578046e4f2e3c46f81de5902bb02fd

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

ff540a3Corrected error in ChartFunction._lmul_ , _rmul_. Still a problem with sympy _eq_

comment:50 Changed 14 months ago by git

  • Commit changed from ff540a3994578046e4f2e3c46f81de5902bb02fd to d0cbda9771f7a2f084331fa5c2101496ad1f4f4c

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

9962353Changed __eq__ for chartfunction in sympy case
14b1c5220204: more doctests
df57f9eMerge branch 'u/rws/20204' of git://trac.sagemath.org/sage into public/22801-manifold-sympy
9169924Merge branch 'develop' into t/24006/24006
d43419f24006: bump sympy patchlevel
d0cbda9Merge branch 'u/rws/24006' of git://trac.sagemath.org/sage into public/22801-manifold-sympy

comment:51 Changed 14 months ago by mmancini

  • Status changed from needs_work to needs_review

comment:52 Changed 14 months ago by rws

  • Milestone changed from sage-8.0 to sage-8.1
  • Reviewers changed from Eric Gourgoulhon to Eric Gourgoulhon, Ralf Stephan
  • Status changed from needs_review to positive_review

I cannot confirm the one patchbot failure. Most of this was reviewed by Eric, and the rest by me, LGTM.

comment:53 Changed 14 months ago by egourgoulhon

  • Status changed from positive_review to needs_review

comment:54 Changed 14 months ago by egourgoulhon

Thank you Ralf but I would like to check a few more things before the final positive review

comment:55 Changed 14 months ago by git

  • Commit changed from d0cbda9771f7a2f084331fa5c2101496ad1f4f4c to b1f6587e44043135087d8da33da7ce1ce6cfb1f9

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

b1f6587Allow for SymPy computation of metric determinant

comment:56 Changed 14 months ago by egourgoulhon

The above commit fixes the computation of metric determinants when sympy is the calculus method. With it, most of the manifold tutorial passes with all calculations performed with SymPy: http://nbviewer.jupyter.org/github/sagemanifolds/SageManifolds/blob/master/Worksheets/v1.1/SM_tutorial_sympy.ipynb.

I have still to check a few extra things to complete the review.

comment:57 follow-up: Changed 14 months ago by rllozes

Grep finds a few mentions of 'manifolds.coord_func' in commentary:

differentiable/scalarfield.py: :class:~sage.manifolds.coord_func.CoordFunction:: differentiable/affine_connection.py: :class:~sage.manifolds.coord_func.CoordFunction (of the subclass differentiable/affine_connection.py: :class:~sage.manifolds.coord_func_symb.CoordFunctionSymb in the current differentiable/affine_connection.py: :class:~sage.manifolds.coord_func.CoordFunction differentiable/chart.py: (cf. :class:~sage.manifolds.coord_func.CoordFunction) differentiable/chart.py: (cf. :class:~sage.manifolds.coord_func.CoordFunction) differentiable/diff_map.py: :class:~sage.manifolds.coord_func.CoordFunction

comment:58 Changed 14 months ago by git

  • Commit changed from b1f6587e44043135087d8da33da7ce1ce6cfb1f9 to 5e2da17ca685852b6d0802cd4705a1e70fe2e123

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

5e2da17Purged references to sage.manifolds.coord_func.CoordFunction

comment:59 in reply to: ↑ 57 Changed 14 months ago by mmancini

Replying to rllozes:

I removed the old references, thanks

Grep finds a few mentions of 'manifolds.coord_func' in commentary:

differentiable/scalarfield.py: :class:~sage.manifolds.coord_func.CoordFunction:: differentiable/affine_connection.py: :class:~sage.manifolds.coord_func.CoordFunction (of the subclass differentiable/affine_connection.py: :class:~sage.manifolds.coord_func_symb.CoordFunctionSymb in the current differentiable/affine_connection.py: :class:~sage.manifolds.coord_func.CoordFunction differentiable/chart.py: (cf. :class:~sage.manifolds.coord_func.CoordFunction) differentiable/chart.py: (cf. :class:~sage.manifolds.coord_func.CoordFunction) differentiable/diff_map.py: :class:~sage.manifolds.coord_func.CoordFunction

comment:60 Changed 14 months ago by git

  • Commit changed from 5e2da17ca685852b6d0802cd4705a1e70fe2e123 to 31f56d2a81e97f1244cc038db3618e26003713e4

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

92e508bFix LaTeX display of continuous maps with SymPy expressions
31f56d2Allow arithmetics of Components with SymPy numbers

comment:61 Changed 14 months ago by egourgoulhon

With the above two commits, all the S2 notebook passes with SymPy: http://nbviewer.jupyter.org/github/sagemanifolds/SageManifolds/blob/master/Worksheets/v1.1/SM_sphere_S2_sympy.ipynb except for the 3D graphics of the tangent vector field to the loxodrome (commented cell 193); this is because Sage's matrix constructor (sage.matrix.constructor.prepare) cannot take SymPy numbers (sympy.core.numbers.Float) as elements: it generates the error

TypeError: unable to find a common ring for all elements

The need for such a matrix arises from a change of basis in some tangent space to the sphere, where all tensor components are expressed in terms of SymPy floats (because of the sampling for the drawing). Probably some workaround can be found.

Besides, I am still checking a few other things...

comment:62 Changed 14 months ago by git

  • Commit changed from 31f56d2a81e97f1244cc038db3618e26003713e4 to 426f98c66cff2a79268e385d4239a02ff05e9ec3

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

426f98cAnother series of reviewer tweaks for SymPy calculus on manifolds

comment:63 Changed 14 months ago by egourgoulhon

The above commit mostly improves the documentation. In particular, it adds the missing item calc_method in some INPUT sections and slightly improves some doctests.

Last edited 14 months ago by egourgoulhon (previous) (diff)

comment:64 Changed 14 months ago by git

  • Commit changed from 426f98c66cff2a79268e385d4239a02ff05e9ec3 to 8ffa9c486af89941311b8f922a4a0893107e176b

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

8ffa9c4Improve documentation for SymPy calculus on manifolds

comment:65 Changed 14 months ago by egourgoulhon

I am almost done checking the documentation; shall finish tomorrow...

comment:66 follow-up: Changed 14 months ago by rllozes

On my 3-manifold, I observe the following timings:

# One thread takes 317s in Sage, 9311s in SymPy?\\

nabla = g.connection()

# One thread takes 95s in Sage, 1134s in SymPy?\\

LBpsi = nabla(nabla(psi).up(g)).trace(); LBpsi;

# One thread takes 872s in Sage, 36221s in SymPy?\\

Ric_g = g.ricci_scalar()

# One thread takes 482s in Sage, 24220s in SymPy?\\

Cot_g = g.cotton(); Cot_g.parent(); Cot_g.display()

I suggest we take a deeper look after getting the functionality released.

comment:67 in reply to: ↑ 66 Changed 14 months ago by egourgoulhon

Replying to rllozes:

I also noticed that SymPy is slower than Sage's symbolic (in particular in the tutorial and S2 notebooks mentioned in comment:56 and comment:61).

I suggest we take a deeper look after getting the functionality released.

I agree.

Changed 14 months ago by rllozes

comment:68 follow-up: Changed 14 months ago by rllozes

The above attached notebook shows a failure when run with the multiprocessing directive

Parallelism().set(nproc=4)

while computing the Laplace-Beltrami operator acting on a scalar field

nabla(nabla(psi).up(g)).trace()

defined on a manifold having the 'sympy' option set

M.set_calculus_method('sympy')

When multiprocessing is turned off (nproc=1), this notebook completes without error.

I will try to find a quicker-running example.

comment:69 in reply to: ↑ 68 ; follow-up: Changed 14 months ago by egourgoulhon

Replying to rllozes:

The above attached notebook shows a failure when run with the multiprocessing directive

Can you resend the notebook? When I try to open it, I get the error

Unreadable Notebook: /home/eric/sage/8.1.beta8/22801_4threads_failure.ipynb NotJSONError("Notebook does not appear to be JSON:u'\\n{{{id=44|\\n# trac 22801 on top of .....",)

comment:70 Changed 14 months ago by git

  • Commit changed from 8ffa9c486af89941311b8f922a4a0893107e176b to 7a49a7814eb436ca991f7dd02ad64ac03f38de77

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

7a49a78Some tweak of documention of tensor fields with SymPy

comment:71 Changed 14 months ago by egourgoulhon

  • Reviewers changed from Eric Gourgoulhon, Ralf Stephan to Eric Gourgoulhon, Ralf Stephan, Richard Lozes

I've finished to review the ticket. Marco, Ralf or Richard, if you agree with my changes, please set the ticket back to positive review (the parallelization issue raised in comment:68, if confirmed, could be dealt with in a separate ticket).

Thank you very much Marco for your work on this; it will be great to have the SymPy functionaly for manifolds in Sage 8.1!

Last edited 14 months ago by egourgoulhon (previous) (diff)

comment:72 Changed 14 months ago by rllozes

I will abstain, having not read through all the changes. I can say that I am able to successfully build and run all the doctests (ptestlong).

comment:73 Changed 14 months ago by mmancini

Thanks to all for the review. I am going to treat the issue of parallelization (comment:68) which it seems not connencted to sympy.

comment:74 Changed 14 months ago by mmancini

  • Status changed from needs_review to positive_review

Changed 14 months ago by rllozes

Showing failure with computations using sympy and parallelism (proc>1)

comment:75 in reply to: ↑ 69 ; follow-up: Changed 14 months ago by rllozes

Replying to egourgoulhon:

Replying to rllozes:

The above attached notebook shows a failure when run with the multiprocessing directive

Can you resend the notebook? When I try to open it, I get the error

Unreadable Notebook: /home/eric/sage/8.1.beta8/22801_4threads_failure.ipynb NotJSONError("Notebook does not appear to be JSON:u'\\n{{{id=44|\\n# trac 22801 on top of .....",)

Sorry - Please see second attachment. I will still try to find a simpler case.

Last edited 14 months ago by rllozes (previous) (diff)

comment:76 follow-up: Changed 14 months ago by rllozes

  • Status changed from positive_review to needs_work

1) Please remove redundant word "curve" which occurs within the documentation of curve.py and integrated_curve.py. 2) Are we letting the TODO's remain undone for this release? 3) In vectorfield_module.py, two occurrences each of "from the North pole" and "from the South pole" were changed to "from North pole" and "from South pole", respectively, resulting in non-standard English.

comment:77 Changed 13 months ago by git

  • Commit changed from 7a49a7814eb436ca991f7dd02ad64ac03f38de77 to ce4acb44667b5e772bb4bd027ac507e63530b4b6

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

ce4acb4Correct some typo in doc and renable trigonometric simplifications in SymPy calculus on manifolds

comment:78 in reply to: ↑ 75 ; follow-up: Changed 13 months ago by egourgoulhon

Replying to rllozes:

Sorry - Please see second attachment. I will still try to find a simpler case.

Sorry I still cannot read this file: when I run sage -n sagenb and try to open it, I get "500: internal server error".

comment:79 in reply to: ↑ 76 Changed 13 months ago by egourgoulhon

Replying to rllozes:

1) Please remove redundant word "curve" which occurs within the documentation of curve.py and integrated_curve.py.

Done in the above commit.

2) Are we letting the TODO's remain undone for this release?

IMHO yes, because they require substantial work and do not hinder the functionality implemented here.

3) In vectorfield_module.py, two occurrences each of "from the North pole" and "from the South pole" were changed to "from North pole" and "from South pole", respectively, resulting in non-standard English.

Corrected in the above commit; thanks.

Besides the above typos, the last commit restores the trig simplification that was commented out in simplify_chain_generic_sympy and simplify_chain_real_sympy. On general grounds, the SymPy simplification chain should be improved in another ticket.

comment:80 Changed 13 months ago by egourgoulhon

  • Status changed from needs_work to positive_review

comment:81 Changed 13 months ago by rllozes

Finished review of all changed code and docs. I concur with status of positive_review.

Changed 13 months ago by rllozes

comment:82 in reply to: ↑ 78 ; follow-up: Changed 13 months ago by rllozes

Replying to egourgoulhon:

Replying to rllozes:

Sorry - Please see second attachment. I will still try to find a simpler case.

Sorry I still cannot read this file: when I run sage -n sagenb and try to open it, I get "500: internal server error".

I have just uploaded another copy, with a new name, after stripping the output and a few commented-out cells. It should open with the standard upload link in the Sage Notebook. It should run as is, and it should fail after changing Parallelism().set(nproc=1) from 1 to 2.

comment:83 Changed 13 months ago by mmancini

I have the same error, looking into the file I don't see any sage command.

comment:84 in reply to: ↑ 82 Changed 13 months ago by egourgoulhon

Replying to rllozes:

I have just uploaded another copy, with a new name, after stripping the output and a few commented-out cells. It should open with the standard upload link in the Sage Notebook. It should run as is, and it should fail after changing Parallelism().set(nproc=1) from 1 to 2.

This time, I can open the worksheet. Thank you. I am just running it.

comment:85 Changed 13 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict with #24062

comment:86 Changed 13 months ago by git

  • Commit changed from ce4acb44667b5e772bb4bd027ac507e63530b4b6 to e5de46981dbe0b23ce7dbbd1509c53c49ad19663

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

9c4119a16801: Conversion of psi(x,y) to/from SymPy
d5c60e324062: pkg version/chksum
1cbc23e24062: remove obsolote patches; adapt remaining
94f359f24062: doctest fixes
e5de469Merge branch 'u/rws/24062' of git://trac.sagemath.org/sage into public/22801-manifold-sympy

comment:87 Changed 13 months ago by mmancini

  • Status changed from needs_work to needs_review

The merge with #24062 done in the last commit. No problem for me

comment:88 Changed 13 months ago by rws

  • Status changed from needs_review to positive_review

comment:89 Changed 13 months ago by git

  • Commit changed from e5de46981dbe0b23ce7dbbd1509c53c49ad19663 to 1f557696bf5045d4b6b9c73004715385b19e69ca
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

6eb6bdeImprove simplifications in calculus on manifolds when derivatives of symbolic functions are involved
1f55769Merge branch of #22801 into that of #24199

comment:90 Changed 13 months ago by egourgoulhon

  • Dependencies changed from #22802 #24006 #20204 to #22802 #24006 #20204 #24199
  • Status changed from needs_review to positive_review

The above commit simply fixes a conflict with #24199. So I am setting the ticket back to "positive review".

comment:91 Changed 13 months ago by egourgoulhon

  • Dependencies changed from #22802 #24006 #20204 #24199 to #22802 #24006 #20204 #24232

comment:92 Changed 13 months ago by git

  • Commit changed from 1f557696bf5045d4b6b9c73004715385b19e69ca to 87877f05a3908e13507afdc281289853a7e1e946
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

7e5baa5Reimplement simplifications in calculus on manifolds via the expression tree
61d97c2Remove unnecessary __init__ and slightly change in the treatment of 1/sqrt(...)
ec105bbUse of wildcards for pattern search in simplifications regarding calculus on manifolds
87877f0Solve merge conflict of #22801 in the dependency #24232

comment:93 Changed 13 months ago by egourgoulhon

  • Status changed from needs_review to positive_review

The above commit simply solves a new merge conflict that appeared after the replacement of #24199 by #24232 in the dependencies.

comment:94 follow-up: Changed 13 months ago by dimpase

See the following bug report, in case https://groups.google.com/d/msg/sage-devel/3sN7gVTdNuY/U9v70LoDAwAJ

By the way, I wonder why you prefer using pexpect interface to maxima, instead of much faster maxima_calcuclus loadable library interface?

Last edited 13 months ago by dimpase (previous) (diff)

comment:95 in reply to: ↑ 94 ; follow-up: Changed 13 months ago by egourgoulhon

Replying to dimpase:

See the following bug report, in case https://groups.google.com/d/msg/sage-devel/3sN7gVTdNuY/U9v70LoDAwAJ

As you pointed out in https://groups.google.com/d/msg/sage-devel/3sN7gVTdNuY/U9v70LoDAwAJ , this bug is not related to this ticket.

By the way, I wonder why you prefer using pexpect interface to maxima, instead of much faster maxima_calcuclus loadable library interface?

Certainly by lack of knowledge... Thanks for your feedback! Can you elaborate a little bit though? Do you mean it is bad to use expr._maxima_() for expr in SR?

comment:96 in reply to: ↑ 95 Changed 13 months ago by rws

Replying to egourgoulhon:

Certainly by lack of knowledge... Thanks for your feedback! Can you elaborate a little bit though? Do you mean it is bad to use expr._maxima_() for expr in SR?

This uses sage/interfaces/interfaces.py which is pexpect. The libmaxima functionality can be found in sage/interfaces/maxima_lib.py.

comment:97 follow-up: Changed 13 months ago by dimpase

I wonder why _maxima_lib_() is used only in few places in the library code, as opposed to _maxima_(). Should one open a ticket to modify all these places?

Certainly maxima_lib is more robust and ought to be faster on large data of certain type, at least. It runs Maxima in a Python extension module rather than talks to Maxima run as an external process.

comment:98 in reply to: ↑ 97 Changed 13 months ago by egourgoulhon

Replying to dimpase:

I wonder why _maxima_lib_() is used only in few places in the library code, as opposed to _maxima_(). Should one open a ticket to modify all these places?

From what you say about maxima_lib, I would say yes. In particular, _maxima_() is used in various methods of sage.symbolic.expression.Expression, like simplify(), simplify_factorial() and simplify_trig().

comment:100 Changed 13 months ago by rllozes

In the code at https://groups.google.com/d/msg/sage-devel/3sN7gVTdNuY/U9v70LoDAwAJ, if we change the computation method from 'SR' to 'sympy', the same Maxima/ECL bug occurs when computing g.inverse(), where 'g' is the Riemannian metric. Why does the code call the same simplification logic, disregarding the computation method setting as 'sympy'?

comment:101 follow-up: Changed 13 months ago by rws

Please open a new ticket.

comment:102 in reply to: ↑ 101 Changed 13 months ago by egourgoulhon

Replying to rws:

Please open a new ticket.

This is #24290. It was on the todo list anyway (cf. the TODO statement in line 794 of https://git.sagemath.org/sage.git/tree/src/sage/manifolds/utilities.py?id=780e1a477fe81c46d32286174fcb58d53e8cbe4c ).

comment:103 Changed 12 months ago by vbraun

  • Branch changed from public/22801-manifold-sympy to 87877f05a3908e13507afdc281289853a7e1e946
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.