Opened 4 years ago
Closed 3 years ago
#22801 closed enhancement (fixed)
SymPy as optional symbolic method for manifolds
Reported by:  mmancini  Owned by:  

Priority:  major  Milestone:  sage8.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, 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 4 years ago by
 Dependencies set to #22802
comment:2 Changed 4 years ago by
 Branch set to 22801manifoldsympy
comment:3 Changed 4 years ago by
 Branch changed from 22801manifoldsympy to public/22801manifoldsympy
 Commit set to 3da57124a6e7cd88ba7928c0a5bdf67603f321c6
comment:4 Changed 4 years ago by
 Cc egourgoulhon added; Eric Gourgoulhon removed
comment:5 Changed 4 years ago by
 Component changed from symbolics to geometry
comment:6 Changed 4 years ago by
 Commit changed from 3da57124a6e7cd88ba7928c0a5bdf67603f321c6 to 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 4 years ago by
 Commit changed from 3be45473000c9141d38710f71ea99373b3c747c0 to 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 4 years ago by
 Commit changed from 01bc6644d4d2647c465920591ff496108f9bcf67 to b07e5fb080271ce765a1f5d27d6b60089e091989
Branch pushed to git repo; I updated commit sha1. New commits:
b07e5fb  ChartFunction pass almost all tests

comment:9 Changed 4 years ago by
 Commit changed from b07e5fb080271ce765a1f5d27d6b60089e091989 to 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4
comment:10 Changed 4 years ago by
 Description modified (diff)
comment:11 Changed 4 years ago by
 Commit changed from 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4 to 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 3 years ago by
 Commit changed from 7fcf258a2ca94c6138a721bd816ddb0a98e11cb2 to 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 3 years ago by
 Commit changed from 66f73dfedce548c8bc61b14ba5f671d5111ac480 to fdb396baa4a2c5027c7a890b22de675949512669
Branch pushed to git repo; I updated commit sha1. New commits:
fdb396b  Minor changes in chart_func

comment:14 Changed 3 years ago by
 Commit changed from fdb396baa4a2c5027c7a890b22de675949512669 to 8f20b54684118bf5c274c540893d397e45d3171f
Branch pushed to git repo; I updated commit sha1. New commits:
8f20b54  Merge branch 'develop' into public/22801manifoldsympy

comment:15 Changed 3 years ago by
 Reviewers set to Eric Gourgoulhon
 Status changed from new to needs_review
comment:16 Changed 3 years ago by
 Commit changed from 8f20b54684118bf5c274c540893d397e45d3171f to 102a3e124d9c5e6a766cae6352ab1956191dc2c9
Branch pushed to git repo; I updated commit sha1. New commits:
102a3e1  error in docstring

comment:17 Changed 3 years ago by
 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 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 3 years ago by
 Commit changed from 102a3e124d9c5e6a766cae6352ab1956191dc2c9 to 4800d97f9dca68300c1799541b4b208a5b43d023
Branch pushed to git repo; I updated commit sha1. New commits:
4800d97  Added better (I hope) docstring

comment:19 followup: ↓ 20 Changed 3 years ago by
 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 3 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 3 years ago by
 Commit changed from 4800d97f9dca68300c1799541b4b208a5b43d023 to ae47c5d7311287403f0ba44e943cc322c33da315
Branch pushed to git repo; I updated commit sha1. New commits:
ae47c5d  Again changes in docstring

comment:22 Changed 3 years ago by
 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 3 years ago by
 Commit changed from ae47c5d7311287403f0ba44e943cc322c33da315 to 611f5de331fef531e0ac7effd6a88fa70f743103
Branch pushed to git repo; I updated commit sha1. New commits:
611f5de  docstring: again problems with sympy

comment:24 Changed 3 years ago by
 Status changed from needs_work to needs_review
comment:25 Changed 3 years ago by
 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 3 years ago by
 Commit changed from 611f5de331fef531e0ac7effd6a88fa70f743103 to 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 3 years ago by
 Commit changed from dd82a154e88edc0be9246337c7146e7088ab8b77 to 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 3 years ago by
 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 followup: ↓ 30 Changed 3 years ago by
 Commit changed from c1d9e4fec66987ea82a7b22d298839979ed15ac0 to fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf
Branch pushed to git repo; I updated commit sha1. New commits:
fb0fb7e  SymPy on manifolds: first reviewer tweaks (more to come)

comment:30 in reply to: ↑ 29 ; followup: ↓ 31 Changed 3 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 in reply to: ↑ 30 Changed 3 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 3 years ago by
 Commit changed from fb0fb7e66395268a1c3e7d9abbf8b5613bf813bf to 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 3 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 in reply to: ↑ 33 Changed 3 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 3 years ago by
 Commit changed from f985a2f3fed328b8b008f53afb080691d32c64b7 to c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f
Branch pushed to git repo; I updated commit sha1. New commits:
c8b3bdc  ChartFunction: modification of _symplify

comment:36 Changed 3 years ago by
 Commit changed from c8b3bdcd62d9a864ecf3236524c8b22902cc1d3f to 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 3 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 3 years ago by
 Commit changed from e1052fab39827595b670bb4d4b5651102781fb3b to 124c582421b49afc38cdd565d0057031c347b044
Branch pushed to git repo; I updated commit sha1. New commits:
124c582  Improve SymPy documentation in sage/manifolds/scalarfield.py

comment:39 Changed 3 years ago by
 Commit changed from 124c582421b49afc38cdd565d0057031c347b044 to 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 3 years ago by
 Commit changed from eea0070a852b76cee592047307bb273172e2dc71 to 07bef4df2312f4413a05e2808d3ddf33b80e1195
Branch pushed to git repo; I updated commit sha1. New commits:
07bef4d  Changing docstring for test results in Sympy

comment:41 Changed 3 years ago by
 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 3 years ago by
 Commit changed from e5ea40e52531f2b1d063df2000b786ebc005dde3 to 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 3 years ago by
 Dependencies changed from #22802 to #22802 #24006
comment:44 Changed 3 years ago by
Better base everything on #20204 as it contains all SymPy Interface improvements, ie abstract functions.
comment:45 Changed 3 years ago by
 Commit changed from 80f67852937d2a2ebffaaf7d0b8318dd9e5847b9 to 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 3 years ago by
 Dependencies changed from #22802 #24006 to #22802 #24006 #20204
comment:47 Changed 3 years ago by
 Commit changed from 952970d2b293ad801ced530645a77cea7dc6d9bc to 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 3 years ago by
 Status changed from needs_review to needs_work
comment:49 Changed 3 years ago by
 Commit changed from 1ff225cce1ffa1f84664bc58002099b883a2fef5 to 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 3 years ago by
 Commit changed from ff540a3994578046e4f2e3c46f81de5902bb02fd to 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 3 years ago by
 Status changed from needs_work to needs_review
comment:52 Changed 3 years ago by
 Milestone changed from sage8.0 to sage8.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 3 years ago by
 Status changed from positive_review to needs_review
comment:54 Changed 3 years ago by
Thank you Ralf but I would like to check a few more things before the final positive review
comment:55 Changed 3 years ago by
 Commit changed from d0cbda9771f7a2f084331fa5c2101496ad1f4f4c to b1f6587e44043135087d8da33da7ce1ce6cfb1f9
Branch pushed to git repo; I updated commit sha1. New commits:
b1f6587  Allow for SymPy computation of metric determinant

comment:56 Changed 3 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 3 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 3 years ago by
 Commit changed from b1f6587e44043135087d8da33da7ce1ce6cfb1f9 to 5e2da17ca685852b6d0802cd4705a1e70fe2e123
Branch pushed to git repo; I updated commit sha1. New commits:
5e2da17  Purged references to sage.manifolds.coord_func.CoordFunction

comment:59 in reply to: ↑ 57 Changed 3 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 3 years ago by
 Commit changed from 5e2da17ca685852b6d0802cd4705a1e70fe2e123 to 31f56d2a81e97f1244cc038db3618e26003713e4
comment:61 Changed 3 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 3 years ago by
 Commit changed from 31f56d2a81e97f1244cc038db3618e26003713e4 to 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 3 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 3 years ago by
 Commit changed from 426f98c66cff2a79268e385d4239a02ff05e9ec3 to 8ffa9c486af89941311b8f922a4a0893107e176b
Branch pushed to git repo; I updated commit sha1. New commits:
8ffa9c4  Improve documentation for SymPy calculus on manifolds

comment:65 Changed 3 years ago by
I am almost done checking the documentation; shall finish tomorrow...
comment:66 followup: ↓ 67 Changed 3 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 in reply to: ↑ 66 Changed 3 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 3 years ago by
comment:68 followup: ↓ 69 Changed 3 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 in reply to: ↑ 68 ; followup: ↓ 75 Changed 3 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 3 years ago by
 Commit changed from 8ffa9c486af89941311b8f922a4a0893107e176b to 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 3 years ago by
 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!
comment:72 Changed 3 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 3 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 3 years ago by
 Status changed from needs_review to positive_review
comment:75 in reply to: ↑ 69 ; followup: ↓ 78 Changed 3 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 3 years ago by
 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 nonstandard English.
comment:77 Changed 3 years ago by
 Commit changed from 7a49a7814eb436ca991f7dd02ad64ac03f38de77 to 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 in reply to: ↑ 75 ; followup: ↓ 82 Changed 3 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 in reply to: ↑ 76 Changed 3 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 3 years ago by
 Status changed from needs_work to positive_review
comment:81 Changed 3 years ago by
Finished review of all changed code and docs. I concur with status of positive_review.
Changed 3 years ago by
comment:82 in reply to: ↑ 78 ; followup: ↓ 84 Changed 3 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 3 years ago by
I have the same error, looking into the file I don't see any sage command.
comment:84 in reply to: ↑ 82 Changed 3 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:85 Changed 3 years ago by
 Status changed from positive_review to needs_work
Merge conflict with #24062
comment:86 Changed 3 years ago by
 Commit changed from ce4acb44667b5e772bb4bd027ac507e63530b4b6 to 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 3 years ago by
 Status changed from needs_work to needs_review
The merge with #24062 done in the last commit. No problem for me
comment:88 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:89 Changed 3 years ago by
 Commit changed from e5de46981dbe0b23ce7dbbd1509c53c49ad19663 to 1f557696bf5045d4b6b9c73004715385b19e69ca
 Status changed from positive_review to needs_review
comment:90 Changed 3 years ago by
 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 3 years ago by
 Dependencies changed from #22802 #24006 #20204 #24199 to #22802 #24006 #20204 #24232
comment:92 Changed 3 years ago by
 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:
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 3 years ago by
 Status changed from needs_review to positive_review
comment:94 followup: ↓ 95 Changed 3 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 in reply to: ↑ 94 ; followup: ↓ 96 Changed 3 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 in reply to: ↑ 95 Changed 3 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 3 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 in reply to: ↑ 97 Changed 3 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 3 years ago by
See #17753 and https://trac.sagemath.org/wiki/symbolics/maxima for a summary.
comment:100 Changed 3 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:101 followup: ↓ 102 Changed 3 years ago by
Please open a new ticket.
comment:102 in reply to: ↑ 101 Changed 3 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 3 years ago by
 Branch changed from public/22801manifoldsympy to 87877f05a3908e13507afdc281289853a7e1e946
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Added sympy symbolic method for manifolds.
merged with develop