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

enhance the integration by using also giac and sympy #27958

Closed
fchapoton opened this issue Jun 9, 2019 · 61 comments
Closed

enhance the integration by using also giac and sympy #27958

fchapoton opened this issue Jun 9, 2019 · 61 comments

Comments

@fchapoton
Copy link
Contributor

CC: @mwageringel @sagetrac-jakobkroeker @kcrisman @pjbruin @rwst @seblabbe @slel @sagetrac-tmonteil @videlec @vinklein @zimmermann6 @mforets @mezzarobba

Component: symbolics

Keywords: integration

Author: Frédéric Chapoton

Branch: d4371e3

Reviewer: Thierry Monteil

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

@fchapoton fchapoton added this to the sage-8.8 milestone Jun 9, 2019
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/27958

@fchapoton
Copy link
Contributor Author

New commits:

3ddd815enhance our integration capacities by using maxima, giac and sympy

@fchapoton
Copy link
Contributor Author

Commit: 3ddd815

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

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

e297e21trac 27958 polishing the details

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

Changed commit from 3ddd815 to e297e21

@fchapoton
Copy link
Contributor Author

Changed keywords from none to integration

@fchapoton
Copy link
Contributor Author

comment:5

sometimes sympy takes ages, so that's why it comes last:

sage: integrate(sqrt(1-m*sin(x)^2),x,algorithm='sympy')
... hangs ...

@fchapoton
Copy link
Contributor Author

comment:6

some failing doctests, work in progress

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

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

4a9fbbbtrac 27958 fixing doctests

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

Changed commit from e297e21 to 4a9fbbb

@fchapoton
Copy link
Contributor Author

comment:8

This seems to work well enough. Waiting for the patchbots' green lights.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

comment:9

Having more integration algorithms tried by default is a very good move. However, doctests should get more analyzed:

This part of the patch looks weird:

-can do, but Sage currently can't do::
+can do, but Sage currently cannot do::
 
-    sage: integrate(log(x)*exp(-x^2), x)        # todo -- Mathematica can do this
-    integrate(e^(-x^2)*log(x), x)
+    sage: integrate(log(x)*exp(-x^2), x)
+    1/2*sqrt(pi)*erf(x)*log(x) - x*hypergeometric((1/2, 1/2), (3/2, 3/2), -x^2)

Even if Sage is not able to perform the nice simplifications, this looks pretty correct (though i am not analyst):

sage: f = 1/2*sqrt(pi)*erf(x)*log(x) - x*hypergeometric((1/2, 1/2), (3/2, 3/2), -x^2)
sage: g =  f.derivative(x)
sage: h = g - log(x)*exp(-x^2)
sage: h.full_simplify()
1/18*(4*x^3*hypergeometric((3/2, 3/2), (5/2, 5/2), -x^2) - 18*x*hypergeometric((1/2, 1/2), (3/2, 3/2), -x^2) + 9*sqrt(pi)*erf(x))/x
sage: h.plot(x,0,1)

That function looks like the zero function ?

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

Reviewer: Thierry Monteil

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

comment:10

Actually, mathematica gives a very similar result : https://www.wolframalpha.com/input/?i=integrate+log(x)*exp(-x%5E2)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

Changed commit from 4a9fbbb to 8eaf4c1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

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

8eaf4c1trac 27958 some details in doc

@fchapoton
Copy link
Contributor Author

comment:12

thx, I have modified this part of the doc

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

comment:13

Also, in

+Todo, Mathematica can do this and gets `\pi^2/15`::
 
     sage: integrate(log(1+sqrt(1+4*x)/2)/x, x, 0, 1)
     Traceback (most recent call last):
     ...
     ValueError: Integral is divergent.

Well, the integral is divergent, as the function behaves like log(3/2)/x around zero. And i bet mathematica never returned pi^2/15, maybe the original example was modified along the time without noticing.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

comment:14

Some other remarks:

  • when you write that Sage can "now" compute some integral, it might be good to refer to :trac:`27958` so that interested people could see how and where the issue was fixed.
  • when you add some conversions dict that are not indirectly tested with the examples, it might be good to doctest them explicitely (i am thinking in particular to the sign conversion for fricas, but maybe there are other that you added without the need to let the new example work, but only you can track those without effort).
  • why not also adding fricas to the tried integrators ? Though it is optional, it could be nice to have it implicitely used when the user has it installed. This may be part of another ticket though.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jun 9, 2019

comment:15

Let me CC @zimmermann6 since some doctests in the Sage book are modified, so that perhaps the example of exp(−x^2 )*ln(x) might not be good anymore to illustrate numerical integration as a fallback.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

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

406e272trac 27958 doc polishing

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 9, 2019

Changed commit from 8eaf4c1 to 406e272

@fchapoton
Copy link
Contributor Author

comment:17

Thx, I have made the suggested changes.

Let us keep the possibility of also adding the fricas integrator for another ticket.

@zimmermann6
Copy link

comment:40

Replying to @mezzarobba:

As one of the authors, that's fine with me, but I'm not the main author of the chapter, and I think the author with the clearest idea of what to do and not to do with these doctests is Paul (already Cc:ed). Paul, if you are reading this, is it okay with you to remove the test?

since it was decided in ticket #27958, comment [comment:58] (while the book was already in print by SIAM), not to accept doctests that do not pass in Python3 (while the book was publicly available since a long time, and nobody asked before for the doctests to be compatible with Python 3), the doctests in Sage do no longer correspond to those in the book, and I have decided not to spend any more time on this.

In summary: do whatever you want, those doctests are no more relevant for the french book.

@videlec
Copy link
Contributor

videlec commented Jun 28, 2019

comment:41

Replying to @zimmermann6:

Replying to @mezzarobba:

As one of the authors, that's fine with me, but I'm not the main author of the chapter, and I think the author with the clearest idea of what to do and not to do with these doctests is Paul (already Cc:ed). Paul, if you are reading this, is it okay with you to remove the test?

[...] in ticket #27958, comment [comment:58] [...]

There is no comment number 58 on ticket #27958!

@zimmermann6
Copy link

comment:42

sorry that was ticket #23572

@videlec
Copy link
Contributor

videlec commented Jun 28, 2019

comment:43

Replying to @zimmermann6:

sorry that was ticket #23572

I did not know about what happend with this ticket. It is indeed unfortunate... the switch to Python 3 is a long and painful route.

But the matter under discussion here is of different nature. The single test under discussion is

N(integrate(exp(-x^2)*log(x), x, 17, 42))

There is a positive change of behavior since Sage will now symbolically integrate exp(-x^2)*log(x). As the doctest was used to illustrate the fact that numerical integration was possible on unevaluated integrals it would not make sense anymore. It is rather the book that has to be adapted here (contrarily to #23572 where the doctest framework should have been adapted).

Isn't there a document that explains what has changed between the available version of the book and the current Sage version? I think this would be very useful.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2019

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d4371e3enhance our integration capacities by using maxima, giac and sympy

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 28, 2019

Changed commit from 43c3bfa to d4371e3

@fchapoton
Copy link
Contributor Author

comment:45

Here is a proposal that keeps the spirit of the modified books' doctests.

@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:47

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jul 9, 2019

comment:48

Regarding builtins, instead of testing that a string is produced:

sage: log_integral(x)._fricas_init_()
'li(x)'

You could (optionally) test that fricas actually handles the function:

sage: log_integral(x)._fricas_()      # optional - fricas
li(x)

as you did for giac.

@fchapoton
Copy link
Contributor Author

comment:49

Well, I prefer the first test, because Fricas is optional.

edit

I could change that if you insist, but this is really not the core of the matter.

@sagetrac-tmonteil

This comment has been minimized.

@sagetrac-tmonteil
Copy link
Mannequin

sagetrac-tmonteil mannequin commented Jul 19, 2019

comment:51

The thing is that this test does not test anything, you could change with any string not representing a fricas function, it will work as well.

Anyway, i see that there are other similar fricas functions that are not tested, so let us postpone that for a dedicated ticket.

@sagetrac-tmonteil

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Jul 20, 2019

Changed branch from u/chapoton/27958 to d4371e3

@egourgoulhon
Copy link
Member

comment:53

See #28964 for a possible follow up.

@egourgoulhon
Copy link
Member

Changed commit from d4371e3 to none

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

9 participants