Opened 6 years ago
Closed 5 years ago
#22801 closed enhancement (fixed)
SymPy as optional symbolic method for manifolds
Reported by:  Marco Mancini  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  geometry  Keywords:  
Cc:  Eric Gourgoulhon  Merged in:  
Authors:  Marco Mancini  Reviewers:  Eric Gourgoulhon, Ralf Stephan, Richard Lozes 
Report Upstream:  N/A  Work issues:  
Branch:  87877f0 (Commits, GitHub, GitLab)  Commit:  87877f05a3908e13507afdc281289853a7e1e946 
Dependencies:  #22802 #24006 #20204 #24232  Stopgaps: 
Description (last modified by )
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)
Change History (106)
comment:1 Changed 6 years ago by
Dependencies:  → #22802 

comment:2 Changed 6 years ago by
Branch:  → 22801manifoldsympy 

comment:3 Changed 6 years ago by
Branch:  22801manifoldsympy → public/22801manifoldsympy 

Commit:  → 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 
comment:4 Changed 6 years ago by
Cc:  Eric Gourgoulhon added; Eric Gourgoulhon removed 

comment:5 Changed 6 years ago by
Component:  symbolics → geometry 

comment:6 Changed 6 years ago by
Commit:  3da57124a6e7cd88ba7928c0a5bdf67603f321c6 → 3be45473000c9141d38710f71ea99373b3c747c0 

Branch pushed to git repo; I updated commit sha1. New commits:
3be4547  created the class ChartFunction which will replace the CoordFunction. also the singleton to manage symbolic method was created

comment:7 Changed 6 years ago by
Commit:  3be45473000c9141d38710f71ea99373b3c747c0 → 01bc6644d4d2647c465920591ff496108f9bcf67 

Branch pushed to git repo; I updated commit sha1. New commits:
01bc664  Merge branch 'public/22801manifoldsympy' of git://trac.sagemath.org/sage into public/22801manifoldsympy

comment:8 Changed 5 years ago by
Commit:  01bc6644d4d2647c465920591ff496108f9bcf67 → b07e5fb080271ce765a1f5d27d6b60089e091989 

Branch pushed to git repo; I updated commit sha1. New commits:
b07e5fb  ChartFunction pass almost all tests

comment:9 Changed 5 years ago by
Commit:  b07e5fb080271ce765a1f5d27d6b60089e091989 → 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4 

comment:10 Changed 5 years ago by
Description:  modified (diff) 

comment:11 Changed 5 years ago by
Commit:  1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4 → 7fcf258a2ca94c6138a721bd816ddb0a98e11cb2 

Branch pushed to git repo; I updated commit sha1. New commits:
7fcf258  passing all test if src/sage/symbolic/expression_conversions.py and changed as in #22802

comment:12 Changed 5 years ago by
Commit:  7fcf258a2ca94c6138a721bd816ddb0a98e11cb2 → 66f73dfedce548c8bc61b14ba5f671d5111ac480 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
068d495  Merge branch 'public/22802_sympy_abstact_function' of git://trac.sagemath.org/sage into public/22802_sympy_abstact_function

078532f  docstring modifications

67f7f46  integrated modifications to composition. But an error remains in french_book.recequadiff tests

1b5227d  Error in french_book.recequadiff tests corrected

018d753  Merge 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

0be1d90  Merge branch 'public/23496_patch_sympy_abstract_function' into 22802_sympy_abstract_function

66f73df  Merge branch 'public/22802_sympy_abstract_function' of git://trac.sagemath.org/sage into public/22801manifoldsympy

comment:13 Changed 5 years ago by
Commit:  66f73dfedce548c8bc61b14ba5f671d5111ac480 → fdb396baa4a2c5027c7a890b22de675949512669 

Branch pushed to git repo; I updated commit sha1. New commits:
fdb396b  Minor changes in chart_func

comment:14 Changed 5 years ago by
Commit:  fdb396baa4a2c5027c7a890b22de675949512669 → 8f20b54684118bf5c274c540893d397e45d3171f 

Branch pushed to git repo; I updated commit sha1. New commits:
8f20b54  Merge branch 'develop' into public/22801manifoldsympy

comment:15 Changed 5 years ago by
Reviewers:  → Eric Gourgoulhon 

Status:  new → needs_review 
comment:16 Changed 5 years ago by
Commit:  8f20b54684118bf5c274c540893d397e45d3171f → 102a3e124d9c5e6a766cae6352ab1956191dc2c9 

Branch pushed to git repo; I updated commit sha1. New commits:
102a3e1  error in docstring

comment:17 Changed 5 years ago by
Status:  needs_review → 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 3dimensional 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 5 years ago by
Commit:  102a3e124d9c5e6a766cae6352ab1956191dc2c9 → 4800d97f9dca68300c1799541b4b208a5b43d023 

Branch pushed to git repo; I updated commit sha1. New commits:
4800d97  Added better (I hope) docstring

comment:19 followup: 20 Changed 5 years ago by
Status:  needs_work → needs_review 

I hope that now the docstring is better (and less liberal)
comment:20 Changed 5 years ago by
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 5 years ago by
Commit:  4800d97f9dca68300c1799541b4b208a5b43d023 → ae47c5d7311287403f0ba44e943cc322c33da315 

Branch pushed to git repo; I updated commit sha1. New commits:
ae47c5d  Again changes in docstring

comment:22 Changed 5 years ago by
Status:  needs_review → needs_work 

According to the patchbots, there are still failing doctests in tensorfield.py
and the doc does not build.
comment:23 Changed 5 years ago by
Commit:  ae47c5d7311287403f0ba44e943cc322c33da315 → 611f5de331fef531e0ac7effd6a88fa70f743103 

Branch pushed to git repo; I updated commit sha1. New commits:
611f5de  docstring: again problems with sympy

comment:24 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:25 Changed 5 years ago by
Status:  needs_review → needs_work 

The last commit fixes the doc build, but not the doctest failures in tensorfield.py
.
comment:26 Changed 5 years ago by
Commit:  611f5de331fef531e0ac7effd6a88fa70f743103 → dd82a154e88edc0be9246337c7146e7088ab8b77 

Branch pushed to git repo; I updated commit sha1. New commits:
dd82a15  chart_func : changing again display depending on calculus method

comment:27 Changed 5 years ago by
Commit:  dd82a154e88edc0be9246337c7146e7088ab8b77 → c1d9e4fec66987ea82a7b22d298839979ed15ac0 

Branch pushed to git repo; I updated commit sha1. New commits:
c1d9e4f  chart_func : changing again display depending on calculus method. All test passed

comment:28 Changed 5 years ago by
Status:  needs_work → 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 followup: 30 Changed 5 years ago by
Commit:  c1d9e4fec66987ea82a7b22d298839979ed15ac0 → fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf 

Branch pushed to git repo; I updated commit sha1. New commits:
fb0fb7e  SymPy on manifolds: first reviewer tweaks (more to come)

comment:30 followup: 31 Changed 5 years ago by
Replying to git:
Branch pushed to git repo; I updated commit sha1. New commits:
fb0fb7e SymPy 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 Changed 5 years ago by
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/\dvI 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 5 years ago by
Commit:  fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf → f985a2f3fed328b8b008f53afb080691d32c64b7 

Branch pushed to git repo; I updated commit sha1. New commits:
f985a2f  SymPy on manifolds: second reviewer tweaks (a slight more to come)

comment:33 followup: 34 Changed 5 years ago by
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 methodChartFunction._simplify
? I mean, ifexpression
is notself.expr(method=calc_method)
, why not calling directly thesimplify
method ofCalculusMethod
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 Changed 5 years ago by
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 methodChartFunction._simplify
? I mean, ifexpression
is notself.expr(method=calc_method)
, why not calling directly thesimplify
method ofCalculusMethod
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) <ipythoninput583503284e242> in <module>() > 1 g=f+Integer(1)/Integer(2) /home/mmancini/Sagelast/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 5 years ago by
Commit:  f985a2f3fed328b8b008f53afb080691d32c64b7 → c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f 

Branch pushed to git repo; I updated commit sha1. New commits:
c8b3bdc  ChartFunction: modification of _symplify

comment:36 Changed 5 years ago by
Commit:  c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f → e1052fab39827595b670bb4d4b5651102781fb3b 

Branch pushed to git repo; I updated commit sha1. New commits:
e1052fa  Use of SymPy subs when relevant in ChartFunction.__call__

comment:37 Changed 5 years ago by
The above commit
 makes
_simplify
a mere shortcut toCalculusMethod.simplify
inChartFunction
 introduces the data member
._chart
in classChartFunction
 uses SymPy substitutions (instead of forcing a conversion to
SR
) inChartFunction.__call__
when dealing with SymPy expressions  makes sure that
simplify_chain_real_sympy
returns a SymPy object  fixes the issue of coercion of
QQ
toChartFunctionRing
comment:38 Changed 5 years ago by
Commit:  e1052fab39827595b670bb4d4b5651102781fb3b → 124c582421b49afc38cdd565d0057031c347b044 

Branch pushed to git repo; I updated commit sha1. New commits:
124c582  Improve SymPy documentation in sage/manifolds/scalarfield.py

comment:39 Changed 5 years ago by
Commit:  124c582421b49afc38cdd565d0057031c347b044 → eea0070a852b76cee592047307bb273172e2dc71 

Branch pushed to git repo; I updated commit sha1. New commits:
eea0070  Change in SymPy conversion function of CalculusMethod + use of SymPy variable in ChartFunction.diff when relevant

comment:40 Changed 5 years ago by
Commit:  eea0070a852b76cee592047307bb273172e2dc71 → 07bef4df2312f4413a05e2808d3ddf33b80e1195 

Branch pushed to git repo; I updated commit sha1. New commits:
07bef4d  Changing docstring for test results in Sympy

comment:41 Changed 5 years ago by
Commit:  07bef4df2312f4413a05e2808d3ddf33b80e1195 → e5ea40e52531f2b1d063df2000b786ebc005dde3 

Branch pushed to git repo; I updated commit sha1. New commits:
e5ea40e   Small error in inverse calculation resolved

comment:42 Changed 5 years ago by
Commit:  e5ea40e52531f2b1d063df2000b786ebc005dde3 → 80f67852937d2a2ebffaaf7d0b8318dd9e5847b9 

Branch pushed to git repo; I updated commit sha1. New commits:
9462954  23990: Convert symbolic relations to SymPy

07f4744  23990: handle unequality

0ed659f  23990: do not evaluate

5f023ea  23990: convert relations from SymPy to Sage, with test

0f596bb  23990: fix patch

479e206  23990: sympy patchlevel bump

3e362ee  24006: SymPy > Sage conversion completely inside Sage

8dd5f88  24006: add missing file, fixes

e0310dd  Merge branch 'u/rws/24006' of git://trac.sagemath.org/sage into public/22801manifoldsympy

80f6785  Resolved the problem of simplification in S2 Chart.set_inverse function

comment:43 Changed 5 years ago by
Dependencies:  #22802 → #22802 #24006 

comment:44 Changed 5 years ago by
Better base everything on #20204 as it contains all SymPy Interface improvements, ie abstract functions.
comment:45 Changed 5 years ago by
Commit:  80f67852937d2a2ebffaaf7d0b8318dd9e5847b9 → 952970d2b293ad801ced530645a77cea7dc6d9bc 

Branch pushed to git repo; I updated commit sha1. New commits:
a68d57f  23923: Interface cases function with SymPy's piecewise

b80d98c  23923: fix typo

2afa0ee  22566: interface SymPy's ceiling()

9494f6c  22566: add test

8215d37  Merge branch 'develop' into tmp05

7899ea9  20204: convert SymPy abstract functions

b261ec3  23923: fix doctest

5010acf  Merge branch 'u/rws/23923' of git://trac.sagemath.org/sage into t/20204/20204

952970d  Merge branch 'u/rws/20204' of git://trac.sagemath.org/sage into public/22801manifoldsympy

comment:46 Changed 5 years ago by
Dependencies:  #22802 #24006 → #22802 #24006 #20204 

comment:47 Changed 5 years ago by
Commit:  952970d2b293ad801ced530645a77cea7dc6d9bc → 1ff225cce1ffa1f84664bc58002099b883a2fef5 

Branch pushed to git repo; I updated commit sha1. New commits:
1ff225c  Added Subs._sage_. Now differentiation in manifold work. A problem in dispaly in diff_form.py appeared

comment:48 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:49 Changed 5 years ago by
Commit:  1ff225cce1ffa1f84664bc58002099b883a2fef5 → ff540a3994578046e4f2e3c46f81de5902bb02fd 

Branch pushed to git repo; I updated commit sha1. New commits:
ff540a3  Corrected error in ChartFunction._lmul_ , _rmul_. Still a problem with sympy _eq_

comment:50 Changed 5 years ago by
Commit:  ff540a3994578046e4f2e3c46f81de5902bb02fd → d0cbda9771f7a2f084331fa5c2101496ad1f4f4c 

Branch pushed to git repo; I updated commit sha1. New commits:
9962353  Changed __eq__ for chartfunction in sympy case

14b1c52  20204: more doctests

df57f9e  Merge branch 'u/rws/20204' of git://trac.sagemath.org/sage into public/22801manifoldsympy

9169924  Merge branch 'develop' into t/24006/24006

d43419f  24006: bump sympy patchlevel

d0cbda9  Merge branch 'u/rws/24006' of git://trac.sagemath.org/sage into public/22801manifoldsympy

comment:51 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:52 Changed 5 years ago by
Milestone:  sage8.0 → sage8.1 

Reviewers:  Eric Gourgoulhon → Eric Gourgoulhon, Ralf Stephan 
Status:  needs_review → 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 5 years ago by
Status:  positive_review → needs_review 

comment:54 Changed 5 years ago by
Thank you Ralf but I would like to check a few more things before the final positive review
comment:55 Changed 5 years ago by
Commit:  d0cbda9771f7a2f084331fa5c2101496ad1f4f4c → b1f6587e44043135087d8da33da7ce1ce6cfb1f9 

Branch pushed to git repo; I updated commit sha1. New commits:
b1f6587  Allow for SymPy computation of metric determinant

comment:56 Changed 5 years ago by
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 followup: 59 Changed 5 years ago by
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 5 years ago by
Commit:  b1f6587e44043135087d8da33da7ce1ce6cfb1f9 → 5e2da17ca685852b6d0802cd4705a1e70fe2e123 

Branch pushed to git repo; I updated commit sha1. New commits:
5e2da17  Purged references to sage.manifolds.coord_func.CoordFunction

comment:59 Changed 5 years ago by
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 5 years ago by
Commit:  5e2da17ca685852b6d0802cd4705a1e70fe2e123 → 31f56d2a81e97f1244cc038db3618e26003713e4 

comment:61 Changed 5 years ago by
With the above two commits, all the S^{2} 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 5 years ago by
Commit:  31f56d2a81e97f1244cc038db3618e26003713e4 → 426f98c66cff2a79268e385d4239a02ff05e9ec3 

Branch pushed to git repo; I updated commit sha1. New commits:
426f98c  Another series of reviewer tweaks for SymPy calculus on manifolds

comment:63 Changed 5 years ago by
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.
comment:64 Changed 5 years ago by
Commit:  426f98c66cff2a79268e385d4239a02ff05e9ec3 → 8ffa9c486af89941311b8f922a4a0893107e176b 

Branch pushed to git repo; I updated commit sha1. New commits:
8ffa9c4  Improve documentation for SymPy calculus on manifolds

comment:65 Changed 5 years ago by
I am almost done checking the documentation; shall finish tomorrow...
comment:66 followup: 67 Changed 5 years ago by
On my 3manifold, 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 Changed 5 years ago by
Replying to rllozes:
I also noticed that SymPy is slower than Sage's symbolic (in particular in the tutorial and S^{2} notebooks mentioned in comment:56 and comment:61).
I suggest we take a deeper look after getting the functionality released.
I agree.
Changed 5 years ago by
Attachment:  22801_4threads.failure added 

comment:68 followup: 69 Changed 5 years ago by
The above attached notebook shows a failure when run with the multiprocessing directive
Parallelism().set(nproc=4)
while computing the LaplaceBeltrami 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 quickerrunning example.
comment:69 followup: 75 Changed 5 years ago by
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 5 years ago by
Commit:  8ffa9c486af89941311b8f922a4a0893107e176b → 7a49a7814eb436ca991f7dd02ad64ac03f38de77 

Branch pushed to git repo; I updated commit sha1. New commits:
7a49a78  Some tweak of documention of tensor fields with SymPy

comment:71 Changed 5 years ago by
Reviewers:  Eric Gourgoulhon, Ralf Stephan → 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!
comment:72 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
Status:  needs_review → positive_review 

Changed 5 years ago by
Attachment:  22801_proc _gt_1.sws added 

Showing failure with computations using sympy and parallelism (proc>1)
comment:75 followup: 78 Changed 5 years ago by
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.
comment:76 followup: 79 Changed 5 years ago by
Status:  positive_review → 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 nonstandard English.
comment:77 Changed 5 years ago by
Commit:  7a49a7814eb436ca991f7dd02ad64ac03f38de77 → ce4acb44667b5e772bb4bd027ac507e63530b4b6 

Branch pushed to git repo; I updated commit sha1. New commits:
ce4acb4  Correct some typo in doc and renable trigonometric simplifications in SymPy calculus on manifolds

comment:78 followup: 82 Changed 5 years ago by
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 Changed 5 years ago by
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 nonstandard 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 5 years ago by
Status:  needs_work → positive_review 

comment:81 Changed 5 years ago by
Finished review of all changed code and docs. I concur with status of positive_review.
Changed 5 years ago by
Attachment:  22801_proc_eq_1.sws added 

comment:82 followup: 84 Changed 5 years ago by
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 commentedout 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 5 years ago by
I have the same error, looking into the file I don't see any sage command.
comment:84 Changed 5 years ago by
Replying to rllozes:
I have just uploaded another copy, with a new name, after stripping the output and a few commentedout 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:86 Changed 5 years ago by
Commit:  ce4acb44667b5e772bb4bd027ac507e63530b4b6 → e5de46981dbe0b23ce7dbbd1509c53c49ad19663 

Branch pushed to git repo; I updated commit sha1. New commits:
9c4119a  16801: Conversion of psi(x,y) to/from SymPy

d5c60e3  24062: pkg version/chksum

1cbc23e  24062: remove obsolote patches; adapt remaining

94f359f  24062: doctest fixes

e5de469  Merge branch 'u/rws/24062' of git://trac.sagemath.org/sage into public/22801manifoldsympy

comment:87 Changed 5 years ago by
Status:  needs_work → needs_review 

The merge with #24062 done in the last commit. No problem for me
comment:88 Changed 5 years ago by
Status:  needs_review → positive_review 

comment:89 Changed 5 years ago by
Commit:  e5de46981dbe0b23ce7dbbd1509c53c49ad19663 → 1f557696bf5045d4b6b9c73004715385b19e69ca 

Status:  positive_review → needs_review 
comment:90 Changed 5 years ago by
Dependencies:  #22802 #24006 #20204 → #22802 #24006 #20204 #24199 

Status:  needs_review → positive_review 
The above commit simply fixes a conflict with #24199. So I am setting the ticket back to "positive review".
comment:91 Changed 5 years ago by
Dependencies:  #22802 #24006 #20204 #24199 → #22802 #24006 #20204 #24232 

comment:92 Changed 5 years ago by
Commit:  1f557696bf5045d4b6b9c73004715385b19e69ca → 87877f05a3908e13507afdc281289853a7e1e946 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
7e5baa5  Reimplement simplifications in calculus on manifolds via the expression tree

61d97c2  Remove unnecessary __init__ and slightly change in the treatment of 1/sqrt(...)

ec105bb  Use of wildcards for pattern search in simplifications regarding calculus on manifolds

87877f0  Solve merge conflict of #22801 in the dependency #24232

comment:93 Changed 5 years ago by
Status:  needs_review → positive_review 

comment:94 followup: 95 Changed 5 years ago by
See the following bug report, in case https://groups.google.com/d/msg/sagedevel/3sN7gVTdNuY/U9v70LoDAwAJ
By the way, I wonder why you prefer using pexpect interface to maxima, instead of much faster maxima_calcuclus loadable library interface?
comment:95 followup: 96 Changed 5 years ago by
Replying to dimpase:
See the following bug report, in case https://groups.google.com/d/msg/sagedevel/3sN7gVTdNuY/U9v70LoDAwAJ
As you pointed out in https://groups.google.com/d/msg/sagedevel/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 Changed 5 years ago by
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_()
forexpr
inSR
?
This uses sage/interfaces/interfaces.py
which is pexpect. The libmaxima functionality can be found in sage/interfaces/maxima_lib.py
.
comment:97 followup: 98 Changed 5 years ago by
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 Changed 5 years ago by
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:99 Changed 5 years ago by
See #17753 and https://trac.sagemath.org/wiki/symbolics/maxima for a summary.
comment:100 Changed 5 years ago by
In the code at https://groups.google.com/d/msg/sagedevel/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:102 Changed 5 years ago by
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 5 years ago by
Branch:  public/22801manifoldsympy → 87877f05a3908e13507afdc281289853a7e1e946 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Added sympy symbolic method for manifolds.
merged with develop