Opened 5 years ago
Closed 4 years ago
#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, 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 5 years ago by
- Dependencies set to #22802
comment:2 Changed 5 years ago by
- Branch set to 22801-manifold-sympy
comment:3 Changed 5 years ago by
- Branch changed from 22801-manifold-sympy to public/22801-manifold-sympy
- Commit set to 3da57124a6e7cd88ba7928c0a5bdf67603f321c6
comment:4 Changed 5 years ago by
- Cc egourgoulhon added; Eric Gourgoulhon removed
comment:5 Changed 5 years ago by
- Component changed from symbolics to geometry
comment:6 Changed 5 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 5 years ago by
- Commit changed from 3be45473000c9141d38710f71ea99373b3c747c0 to 01bc6644d4d2647c465920591ff496108f9bcf67
Branch pushed to git repo; I updated commit sha1. New commits:
01bc664 | Merge branch 'public/22801-manifold-sympy' of git://trac.sagemath.org/sage into public/22801-manifold-sympy
|
comment:8 Changed 5 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 5 years ago by
- Commit changed from b07e5fb080271ce765a1f5d27d6b60089e091989 to 1e2dc01b1e300fe35e9eb5a96f9fb338558b63e4
comment:10 Changed 5 years ago by
- Description modified (diff)
comment:11 Changed 5 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 5 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/22801-manifold-sympy
|
comment:13 Changed 5 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 5 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/22801-manifold-sympy
|
comment:15 Changed 5 years ago by
- Reviewers set to Eric Gourgoulhon
- Status changed from new to needs_review
comment:16 Changed 5 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 5 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 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 5 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 follow-up: ↓ 20 Changed 5 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 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 changed from 4800d97f9dca68300c1799541b4b208a5b43d023 to 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 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 5 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 5 years ago by
- Status changed from needs_work to needs_review
comment:25 Changed 5 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 5 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 5 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 5 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 follow-up: ↓ 30 Changed 5 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 ; follow-up: ↓ 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 in reply to: ↑ 30 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 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 follow-up: ↓ 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 in reply to: ↑ 33 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) <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 5 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 5 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 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 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 5 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 5 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 5 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 5 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/22801-manifold-sympy
|
80f6785 | Resolved the problem of simplification in S2 Chart.set_inverse function
|
comment:43 Changed 5 years ago by
- Dependencies changed from #22802 to #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 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/22801-manifold-sympy
|
comment:46 Changed 5 years ago by
- Dependencies changed from #22802 #24006 to #22802 #24006 #20204
comment:47 Changed 5 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 5 years ago by
- Status changed from needs_review to needs_work
comment:49 Changed 5 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 5 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/22801-manifold-sympy
|
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/22801-manifold-sympy
|
comment:51 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:52 Changed 5 years ago by
- 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 5 years ago by
- Status changed from positive_review to 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 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 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 follow-up: ↓ 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 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 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 changed from 5e2da17ca685852b6d0802cd4705a1e70fe2e123 to 31f56d2a81e97f1244cc038db3618e26003713e4
comment:61 Changed 5 years ago by
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 5 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 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 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 5 years ago by
I am almost done checking the documentation; shall finish tomorrow...
comment:66 follow-up: ↓ 67 Changed 5 years ago by
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 5 years ago by
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 5 years ago by
comment:68 follow-up: ↓ 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 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: ↓ 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 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 5 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 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 changed from needs_review to positive_review
comment:75 in reply to: ↑ 69 ; follow-up: ↓ 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 follow-up: ↓ 79 Changed 5 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 non-standard English.
comment:77 Changed 5 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 ; follow-up: ↓ 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 in reply to: ↑ 76 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 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 5 years ago by
- Status changed from needs_work to 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
comment:82 in reply to: ↑ 78 ; follow-up: ↓ 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 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 5 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 5 years ago by
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 5 years ago by
- Status changed from positive_review to needs_work
Merge conflict with #24062
comment:86 Changed 5 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/22801-manifold-sympy
|
comment:87 Changed 5 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 5 years ago by
- Status changed from needs_review to positive_review
comment:89 Changed 5 years ago by
- Commit changed from e5de46981dbe0b23ce7dbbd1509c53c49ad19663 to 1f557696bf5045d4b6b9c73004715385b19e69ca
- Status changed from positive_review to needs_review
comment:90 Changed 5 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 5 years ago by
- Dependencies changed from #22802 #24006 #20204 #24199 to #22802 #24006 #20204 #24232
comment:92 Changed 4 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 4 years ago by
- Status changed from needs_review to positive_review
comment:94 follow-up: ↓ 95 Changed 4 years ago by
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?
comment:95 in reply to: ↑ 94 ; follow-up: ↓ 96 Changed 4 years ago by
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 4 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 follow-up: ↓ 98 Changed 4 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 4 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 4 years ago by
See #17753 and https://trac.sagemath.org/wiki/symbolics/maxima for a summary.
comment:100 Changed 4 years ago by
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: ↓ 102 Changed 4 years ago by
Please open a new ticket.
comment:102 in reply to: ↑ 101 Changed 4 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 4 years ago by
- Branch changed from public/22801-manifold-sympy to 87877f05a3908e13507afdc281289853a7e1e946
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Added sympy symbolic method for manifolds.
merged with develop