Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Doctest: Remaining issues with symbolic product #22989

Closed
rwst opened this issue May 13, 2017 · 32 comments
Closed

Doctest: Remaining issues with symbolic product #22989

rwst opened this issue May 13, 2017 · 32 comments

Comments

@rwst
Copy link

rwst commented May 13, 2017

Continued from #17505 this ticket fixes LaTeX, documentation, and doctest issues around the symbolic product.

Depends on #22937

CC: @EmmanuelCharpentier @tscrim

Component: calculus

Author: Ralf Stephan, Emmanuel Charpentier

Branch/Commit: 8be8a0c

Reviewer: Marcelo Forets

Issue created by migration from https://trac.sagemath.org/ticket/22989

@rwst rwst added this to the sage-8.0 milestone May 13, 2017
@rwst
Copy link
Author

rwst commented May 13, 2017

comment:2

BTW, Maxima can give back some "interesting" results:

sage: from sage.calculus.calculus import symbolic_product
sage: symbolic_product(1+(-1)^(x+1)/x,x,1,oo)            
...
ValueError: power::eval(): pow(1, Infinity) is not defined.

@rwst
Copy link
Author

rwst commented May 13, 2017

comment:3

Oops.

@rwst
Copy link
Author

rwst commented May 13, 2017

Branch: u/rws/22989

@rwst
Copy link
Author

rwst commented May 13, 2017

New commits:

e75c12422989: Remaining issues with symbolic product

@rwst
Copy link
Author

rwst commented May 13, 2017

Author: Ralf Stephan

@rwst
Copy link
Author

rwst commented May 13, 2017

Commit: e75c124

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 13, 2017

comment:6

Replying to @rwst:

New commits:

e75c12422989: Remaining issues with symbolic product

Can't pull this branch in mine : I've rebased on 8.0.beta6,and this one has a modification on src/sage/interfaces/maxima_lib.py incompatible with yours.

@rwst
Copy link
Author

rwst commented May 13, 2017

Changed branch from u/rws/22989 to u/rws/22989-1

@rwst
Copy link
Author

rwst commented May 13, 2017

comment:8

I ended up using your branch and resolve the conflict, so this depends on #22937.


Last 10 new commits:

687cc8eDistribute : implement Travis Scrimshaw's suggestion for iterations.
d420ec417505: fix latex, cosmetics
b784a2cMerge branch 'u/rws/implement_symbolic_product' of trac.sagemath.org:sage into distribute
577942317505: fix doctests
4b6e71bMerge branch 'u/rws/implement_symbolic_product' of trac.sagemath.org:sage into distribute
2412f7aDistribute : cosmetics on documentation.
c28097aDistribute : at his request, Travis Crimshaw removed from Author's list.
7aee739Distribute : one last typo (I hope...).
b2b4a0fMerge branch 'develop' into t/22937/distribute
7df31d922989: Remaining issues with symbolic product

@rwst
Copy link
Author

rwst commented May 13, 2017

Dependencies: #22937

@rwst
Copy link
Author

rwst commented May 13, 2017

Changed commit from e75c124 to 7df31d9

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 14, 2017

comment:9

Found a small oversight in latex functions for Function_sum and Function_prod :

sage: latex(sum(sin(X(j)),j,1,p))
{\sum_{j=1}^{p} sin(X(j))}

whereas what is sought is something along the lines of {\sum_{j=1}^{p} \sin\left(X\left(j\right)\right)}

Patch suggestion :

charpent@asus16-ec:/usr/local/sage-8$ git diff
diff --git a/src/sage/functions/other.py b/src/sage/functions/other.py
index aaee96cf87..aafbb697e8 100644
--- a/src/sage/functions/other.py
+++ b/src/sage/functions/other.py
@@ -2617,7 +2617,8 @@ class Function_sum(BuiltinFunction):
             sage: latex(ssum(x^2, x, 1, 10))
             {\sum_{x=1}^{10} x^2}
         """
-        return r"{{\sum_{{{}={}}}^{{{}}} {}}}".format(var, a, b, x)
+        return r"{{\sum_{{{}={}}}^{{{}}} {}}}".format(latex(var), latex(a),
+                                                      latex(b), latex(x))
 
 symbolic_sum = Function_sum()
 
@@ -2664,6 +2665,7 @@ class Function_prod(BuiltinFunction):
             sage: latex(sprod(x^2, x, 1, 10))
             {\prod_{x=1}^{10} x^2}
         """
-        return r"{{\prod_{{{}={}}}^{{{}}} {}}}".format(var, a, b, x)
+        return r"{{\prod_{{{}={}}}^{{{}}} {}}}".format(latex(var), latex(a),
+                                                       latex(b), latex(x))
 
 symbolic_product = Function_prod()

Simple-minded but correct (I think).

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 14, 2017

comment:10

Yet another note :

sum() expands its first argument. I'm not sure that this is pertinent. Compare :

sage: sum((X(j)+Y(j))^2,j,1,p)
sum(X(j)^2 + 2*X(j)*Y(j) + Y(j)^2, j, 1, p)
sage: maxima("sum(((X(j)+Y(j))^2),j,1,p)").sage()
sum((X(j) + Y(j))^2, j, 1, p)

I have implemented (in a private branch), an "expand" option controlling if distribute() should expand its first argument (default) or not (might come in handy in some situations). This allows :

sage: maxima("sum((X(j)+Y(j))^2+Z(j),j,1,p)").sage().distribute()
sum(X(j)^2, j, 1, p) + sum(2*X(j)*Y(j), j, 1, p) + sum(Y(j)^2, j, 1, p) + sum(Z(j), j, 1, p)
sage: maxima("sum((X(j)+Y(j))^2+Z(j),j,1,p)").sage().distribute(expand=False)
sum((X(j) + Y(j))^2, j, 1, p) + sum(Z(j), j, 1, p)

But this currently works only from sum expressions cast from Maxima ; Sage-built sums get expanded volens nolens, as seen above, and the resulting expansions can't be factorized back by factor().

Possible solution : an "expand" keyword argument to sum (default=True) controlling the expansion ? What do you think ?

The same goes, of course, for products.

@fchapoton
Copy link
Contributor

comment:11

Please take take ASAP of the apply/python3 issue introduced in #22937.

@rwst
Copy link
Author

rwst commented May 20, 2017

comment:12

The branch is fine.

@rwst rwst changed the title Remaining issues with symbolic product Doctest: Remaining issues with symbolic product May 20, 2017
@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 21, 2017

Changed branch from u/rws/22989-1 to u/charpent/22989-1

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 21, 2017

Changed author from Ralf Stephan to Ralf Stephan, Emmanuel Charpentier

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 21, 2017

Changed commit from 7df31d9 to 4c3d7f4

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 21, 2017

comment:14

This has been rebased over 8.0.beta7 (which incorporates #22937). Three fixes :

  • Latex generation for symbolic sums and products.
  • A .prod() method for symbolic expressions.
  • A public product() function (I wanted to nmae it prod, but this clashes irreconciliably with the existing prod function for lists and others. I you see how to implement this, you're welcome...).

Passes ptestlong with the usual transient sage -t --long src/sage/homology/simplicial_complex.py failure, which is transient.

==> needs_review

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented May 29, 2017

comment:15

As of the day before yesterday, the patchbots started giving an error in building g2fx that I don't understand a bit...

Ca some kind sould enlighten me on the possible causes (and possible remedies ?)

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

comment:16

some comments:

  • the (optional) use Giac should be just use Giac
  • 'mathematica' - (optional) use Mathematica is missing
  • add a SEEALSO block pointing to the new symbolic_product in the top level prod (misc_c.pyx)

if you don't mind, i can add these minor things myself and review asap.

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Jun 4, 2017

comment:17

Replying to @mforets:

some comments:

  • the (optional) use Giac should be just use Giac

Indeed : giac is now standard...

  • 'mathematica' - (optional) use Mathematica is missing

Indeed.

But I'm not so sure : the Mathematica interface has other (serious) problems, that can be triggered also in sums and products. This, IMHO, is a distinct problem, and should be fixed by someone knowing what it does with Mathematica (I don't...).

Is it reasonable do document a (good) way to use a (flaky) interface ? I let you judge...

  • add a SEEALSO block pointing to the new symbolic_product in the top level prod (misc_c.pyx)

Right...

if you don't mind, i can add these minor things myself and review asap.

Please go ahead ! Do you need me to review your review ?

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

Changed branch from u/charpent/22989-1 to u/mforets/22989-1

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

Changed commit from 4c3d7f4 to 5b8b16c

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

comment:19

done. for mathematica unfortunately i also don't have it so cannot test, but i do think it's good to mention as a valid option.

i'm having an issue with the hold option:

sage: k, n = var('k, n')
sage: sage.calculus.calculus.symbolic_product(k, k, 1, n)
factorial(n)
sage: sage.calculus.calculus.symbolic_product(k, k, 1, n, hold=True)
---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-3-1fc22673b8c7> in <module>()
----> 1 sage.calculus.calculus.symbolic_product(k, k, Integer(1), n, hold=True)

/Users/forets/sage-src/sage/local/lib/python2.7/site-packages/sage/calculus/calculus.pyc in symbolic_product(expression, v, a, b, algorithm, hold)
    868
    869     if hold == True:
--> 870         from sage.functions.other import symbolic_prod as sprod
    871         return sprod(expression, v, a, b)
    872

ImportError: cannot import name symbolic_prod

let me fix it by changing symbolic_prod to symbolic_product in line 870


New commits:

5b8b16cdocstring tweaks

@tscrim
Copy link
Collaborator

tscrim commented Jun 4, 2017

comment:20

Replying to @EmmanuelCharpentier:

Replying to @mforets:

  • 'mathematica' - (optional) use Mathematica is missing

But I'm not so sure : the Mathematica interface has other (serious) problems, that can be triggered also in sums and products. This, IMHO, is a distinct problem, and should be fixed by someone knowing what it does with Mathematica (I don't...).

Is it reasonable do document a (good) way to use a (flaky) interface ? I let you judge...

We are suppose to be supporting an interface to Mathematica, so I think we should document it. Doing so will both increase our user base and help find bugs from people using said interface.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 4, 2017

Changed commit from 5b8b16c to 8be8a0c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 4, 2017

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

8be8a0cfix import in hold option

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

comment:22

symbolic product works and html doc builds, tests in relevant modules pass.

@mforets
Copy link
Mannequin

mforets mannequin commented Jun 4, 2017

Reviewer: Marcelo Forets

@rwst
Copy link
Author

rwst commented Jun 5, 2017

comment:23

Thanks.

@vbraun
Copy link
Member

vbraun commented Jun 12, 2017

Changed branch from u/mforets/22989-1 to 8be8a0c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants