Doctest conversion from SymPy of unevaluated integrals

[eviatarbach]

When SymPy? can't evaluate an integral, such as integrate((log(x)*log(log(x))), x, algorithm='sympy'), it returns "AttributeError?: 'Integral' object has no attribute '_sage_'". It should just return an unevaluated integral, the way it does when Maxima is used.

Another example from #15256:

sage: a = integrate(sin(x)*tan(x), x, algorithm='sympy') 
...

 
/usr/local/sage/sage-5.11/local/lib/python2.7/site-packages/sage/symbolic/integration/external.pyc in sympy_integrator(expression, v, a, b)
     37     else:
     38         result = sympy.integrate(ex, (v, a._sympy_(), b._sympy_()))
---> 39     return result._sage_()
     40
     41 def mma_free_integrator(expression, v, a=None, b=None):
 
AttributeError: 'Integral' object has no attribute '_sage_'
sage: 
sage: 
sage: %debug
> /usr/local/sage/sage-5.11/local/lib/python2.7/site-packages/sage/symbolic/integration/external.py(39)sympy_integrator()
     38         result = sympy.integrate(ex, (v, a._sympy_(), b._sympy_()))
---> 39     return result._sage_()
     40 
 
ipdb> print result
Integral(sin(x)*tan(x), x)

[eviatarbach]

With the patch, integrate(log(x)*log(log(x)), x) is returned.

[asmeurer]

Just looking at the patch, that doesn't look like it does the right thing for definite integrals.

[kcrisman]

This is a dup of #15256. Not sure which one should be closed.

[rws]

The original case is now working in sympy:

sage: integrate(sin(x)*tan(x), x, algorithm='sympy') 
1/2*log(sin(x) + 1) - 1/2*log(sin(x) - 1) - sin(x)

so we need a more complicated one:

sage: integrate(sin(x)*tan(x)/(1-cos(x)), x, algorithm='sympy')
integrate(-sin(x)*tan(x)/(cos(x) - 1), x)

However, comment:7 is right regarding definite integrals:

sage: integrate(sin(x)*tan(x)/(1-cos(x)), x, a, b, algorithm='sympy')
integrate(-sin(x)*tan(x)/(cos(x) - 1), x)

[rws]

It is a matter of either

• adding a _sage_ method to the upstream sympy code in sympy/integrals/integrals.py or
• catching the exception in sage.symbolic.integration.external.sympy_integrator()

EDIT: The problem with the latter is, all sorts of elementary sympy functions have a _sage_ method, so why not add one to sympy.Integral or even do a bulk upgrade of sympy (e.g. gamma._sage_ is missing too)?

Actually, this is ​https://github.com/sympy/sympy/issues/3444

[kcrisman]

Eventually we may want to start looking into switching to SymPy for the default integration method, or possibly trying both by default (not sure how long that would take, though). Before that, we'd probably want to work with #7763, though - and what about #2787? I personally don't know that I like deprecating it, but all of this is part of the same package of taking advantage of the capabilities out there by having a more consistent interface.

[rws]

This Sympy patch completely fixes the issue. A pull request containing it was reported there and waits for review.

​https://github.com/sympy/sympy/pull/8592

diff --git a/sympy/integrals/integrals.py b/sympy/integrals/integrals.py
index 2a3ccfe..ac434c5 100644
--- a/sympy/integrals/integrals.py
+++ b/sympy/integrals/integrals.py
@@ -1070,6 +1070,27 @@ def as_sum(self, n, method="midpoint"):
             result += self.function.subs(sym, xi)
         return result*dx
 
+    def _sage_(self, ):
+        from sage.symbolic.integration.integral import definite_integral, indefinite_integral
+        f, limits = self.function, list(self.limits)
+        limit = limits.pop(-1)
+        if len(limit) >= 2:
+            if len(limit) == 2:
+                x, b = limit
+                a = None
+            else:
+                x, a, b = limit
+            return definite_integral(f._sage_(),
+                                        x._sage_(),
+                                        a._sage_(),
+                                        b._sage_(),
+                                        hold=True)
+        else:
+            x = limit[0]
+            return indefinite_integral(f._sage_(),
+                                       x._sage_(),
+                                       hold=True)
+
 
 @xthreaded
 def integrate(*args, **kwargs):

[kcrisman]

Huh, that looks pretty straightforward. Of course one would want to test it with a variety of round-trips, especially involving infinity or symbolic variables as endpoints. How would it hold up with multivariate functions?

[rws]

Replying to kcrisman:

Huh, that looks pretty straightforward. Of course one would want to test it with a variety of round-trips, especially involving infinity or symbolic variables as endpoints. How would it hold up with multivariate functions?

Well, if that SymPy? object parameter you are talking about has a _sage_ method, it will be called and does the right thing before it's given to the Sage function as parameter. This patch is only about SymPy?'s Integral object itself, not its parameters.

[rws]

I decided to patch our own sympy until that sympy pull request gets pulled.

---

New commits:

​8c345ce

14723: implement Integrate._sage_ in intalled sympy

[git]

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

​cf6f7b1

14723: change patchlevel too

[rws]

Some issues surfaced, see Sympy pull request.

[rws]

Replying to kcrisman:

Eventually we may want to start looking into switching to SymPy for the default integration method, or possibly trying both by default (not sure how long that would take, though). Before that, we'd probably want to work with #7763, though - and what about #2787? I personally don't know that I like deprecating it, but all of this is part of the same package of taking advantage of the capabilities out there by having a more consistent interface.

I see, I just stumbled over the necessity for #2787 for this ticket because of #17507. I'll dupe #17507 and make #2787 a dependency for this one.

[kcrisman]

Interesting! Just be careful - particularly for something so heavily used by non-experts (including their instructors) as integrals, we would really need a deprecation period - see this example for a particularly irksome decision that would have to be made.

[rws]

I take that completely back. I was in error, #17507 is only a misleading error message. The interface is consistent. Sorry.

[kcrisman]

Some issues surfaced, see Sympy pull request.

I assume you mean issues beyond whitespace and tests? We can still include a patch for now if necessary, without just updating upstream.

[rws]

Replying to kcrisman:

Some issues surfaced, see Sympy pull request.

I assume you mean issues beyond whitespace and tests?

Yes, I forgot to handle multiple integrals. SymPy? has implemented the issue of #2787 with its Integrate.

[rws]

Sympy's "antiderivative at" will not have an equivalent in Sage:

sage: sympy.Integral(sympy.sin(x),(x,0)).doit()
-1
sage: integral(sin(x),x,0,hold=True)
integrate(sin(x), x, x, 0)
sage: _.simplify()
cos(x) - 1

EDIT: second problem deleted, wasn't.

[kcrisman]

Sympy's "antiderivative at" will not have an equivalent in Sage:

What is that supposed to be mathematically? Maybe Maxima has something like this that we already translate for them? Do you think there is a way to deal with this by raising an error upon translation back to Sage (if that's appropriate)? Sorry for the questions; each software has a slightly different philosophy.

[git]

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

​099b9f6

14723: handle multiple integrals from SymPy too

[rws]

Replying to kcrisman:

Sympy's "antiderivative at" will not have an equivalent in Sage:

What is that supposed to be mathematically?

The indefinite integral value at the given point. I.e., if F(x) = integral(f(x),x) then F(a) = sympy.Integral(f(x), x, a).

Maybe Maxima has something like this that we already translate for them?

Couldn't find such a thing.

Do you think there is a way to deal with this by raising an error upon translation back to Sage (if that's appropriate)? Sorry for the questions; each software has a slightly different philosophy.

Computation would be 1. compute the indef. integral; 2. if succesful substitute. So not difficult I guess.

[rws]

Replying to rws:

Computation would be 1. compute the indef. integral; 2. if succesful substitute. So not difficult I guess.

However, we are getting unevaluated Integral objects, so nothing would be necessary IF #12438 would not change the parameters via _normalize_integral_input(), so a fix of that function would be necessary as well.

[kcrisman]

The indefinite integral value at the given point.

Yeah, I never know what to do with this - even though there is no "the" indefinite integral most systems just return a convenient one. I think that there is a problem with actually having this syntax, though, because in general there are lots of times (esp. with complicated trig stuff) that the integral you get before simplifying is one thing, the one you get if you simplify or do some trig expansion first is different, and they differ by a non-zero constant! It would be very bad to have

sage: F(x) = complicated stuff
sage: integral(f, x, 0, hold=True).simplify()
0
sage: integral(f.some_simplification_routine(), x, 0, hold=True).simplify()
1/2

See also #13221 for the thought process behind #12438, though hopefully without too much of a hack it would be possible to support both, if that were desired.

[rws]

I would argue there is actually no scenario where Sympy would return such an antiderivative unevaluated when asked from Sage for an integral, just because #12438 rewrites the parameters *before* handing it to Sympy with algorithm='sympy'.

So, we do not want it, we won't get it. Please open an enhancement ticket if you think it's a necessary feature. Please review.

[asmeurer]

The SymPy? integral at a point is used by the differential equation solver in some cases where it would otherwise have to introduce a new constant for the lower limit of integration (or use negative infinity, which might not even converge). There are some examples at ​http://docs.sympy.org/latest/modules/solvers/ode.html. The result always ends up being correct because there always ends up being a + C1 in the solution. The syntax was borrowed from Maple, which uses it for the same purpose. Note that this syntax was introduced (by me) before we had an unevaluated Subs object. If it had existed at the time, I probably would have just used it instead.

[kcrisman]

Minor notes:

• I think the first revision is .p0, not .p1, as Python counts starting at zero. (Very minor note!)
• You should have at least one example for a definite integral (the one in comment:13 is really slow, though).
• Is it possible to have a multiple integral example, or is the point that Sympy might want to translate a multiple integral to Sage, but not vice versa?

Anyway, this is fine and hopefully in the long run will lead to more Sage-Sympy helping each other out, always a good thing. Though I guess I'd feel most comfortable actually merging it once upstream has done so in their unstable, if they have such a concept (could be sage-pending until then).

[git]

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

​af20fad

14723: better testing, patchlevel

[rws]

Of course we didn't need an unsolvable integral for testing this. Setting pending. Can positive be set too?

[kcrisman]

Oh, I guess we don't have to have an undoable one. Though my preference would be to keep it so that it raises a doctest error when Sympy can do it, which will alert us to it :-) as well as dealing with the actual reported issue on the ticket, which is our usual practice. That said, I understand the point of testing the underlying issue. What do you think?

Just waiting on testing this new patch for the other thing you asked about... okay!

[kcrisman]

I do get an unrelated error (also on develop) - ever seen this?

sage -t src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 10566, in sage.symbolic.expression.get_dynamic_class_for_function
Failed example:
    import sagenb.misc.support as s
Expected nothing
Got:
    doctest:14: ImportWarning: Not importing directory '/Users/.../sage/local/lib/python2.7/site-packages/Babel-1.3-py2.7.egg/babel/localedata': missing __init__.py
**********************************************************************

It's probably related to my messing around with sagenb, no worries.

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​4c450f8

14723: the crux of the biscuit is the apostrophe

[kcrisman]

That was a good catch.

[asmeurer]

You can't just test integral(f(x), x), where f is an unevaluated function (Function('f') in SymPy?)? SymPy? will always return an unevaluated integral for that.

[kcrisman]

You can't just test integral(f(x), x), where f is an unevaluated function (Function('f') in SymPy?)? SymPy? will always return an unevaluated integral for that.

Are you saying we should test the integrals that can't be solved to properly test this? I don't think there is a test like this in the current patch.

[rws]

Replying to kcrisman:

You can't just test integral(f(x), x), where f is an unevaluated function (Function('f') in SymPy?)? SymPy? will always return an unevaluated integral for that.

Are you saying we should test the integrals that can't be solved to properly test this? I don't think there is a test like this in the current patch.

Contrary, the code of this ticket is only invoked when SymPy? returns an unevaluated integral. What he suggested was replacing x with f(x) (for clarity I guess).

[asmeurer]

My point is just if you want to have an "unsolvable" integral that is the best one.

[rws]

Oh, that would need a _sage_ method for sympy.core.function.UndefinedFunction as well. Let's do that, for completeness.

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​dd4f7ff

14723: accept undefined functions from SymPy

[kcrisman]

I presume that the latest commit, with itsclass UndefinedFunction(FunctionClass): is the equivalent of one of the changes in the same file at ​the pull request, or does that need to be added upstream as well?

Given that asmeurer has also weighed in here, I think I am happy with accepting this before the pull request upstream formally joins in, though, so I'll give a positive review once I have a chance to test this a bit more (not this morning EST, my apologies).

[rws]

Replying to kcrisman:

... or does that need to be added upstream as well?

Yes, it's now added.

[kcrisman]

Along these lines, we have

sage: integrate(integrate(integrate(f,x),y),z)
integrate(integrate(integrate(f(x, y, z), x), y), z)

but

sage: var('y z')
(y, z)
sage: f = function('f',x,y,z)
sage: f
f(x, y, z)
sage: integrate(integrate(integrate(f,x,algorithm='sympy'),y,algorithm='sympy'),z,algorithm='sympy')
---------------------------------------------------------------------------
<snip>
    690             return f_sympy(*sympy.sympify(g, evaluate=False))
    691         else:
--> 692             raise NotImplementedError("SymPy function '%s' doesn't exist" % f)
    693 
    694 sympy = SympyConverter()

NotImplementedError: SymPy function 'f' doesn't exist
sage: integrate(integrate(integrate(f,x),y),z)

Perhaps it would be good to fix that here, or elsewhere?

But I like that

sage: integrate(integrate(integrate(abs(x),x,algorithm='sympy'),y,algorithm='sympy'),z,algorithm='sympy')
integrate(y*z*abs(x), x)
sage: integrate(integrate(integrate(abs(x*y*z),x,algorithm='sympy'),y,algorithm='sympy'),z,algorithm='sympy')
integrate(integrate(integrate(abs(x*y*z), x), y), z)

so otherwise I think we should be good to go.

[git]

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

​e2f0713

14723: sage-style doctests; conversion of symbolic fns to SymPy

[kcrisman]

Thanks, this is really a much more robust patch now. I'm sure there are still some things we won't get right, but this goes a long ways toward better Sage-Sympy integration. Just waiting on some basic tests for positive review.

[kcrisman]

Glad I ran the tests - this is a good one to have fail, though :-)

**********************************************************************
File "src/sage/symbolic/function.pyx", line 634, in sage.symbolic.function.Function._sympy_init_
Failed example:
    g(x)._sympy_()
Expected:
    Traceback (most recent call last):
    ...
    NotImplementedError: SymPy function 'gg' doesn't exist
Got:
    gg(x)
**********************************************************************

I do love that we code errors in the tests, because it's always gratifying to see when someone has fixed one. (Can you think of another way to test that error branch, incidentally? I have to admit I can't offhand, unless it's a (very unusual!) Sage symbolic function not in Sympy, maybe something number-theoretic, or maybe just a fake function?)

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​880a617

14723: fix doctest

[kcrisman]

Thanks for all the work here.

[rws]

Thanks for the review.

[vbraun]

PSA: Once you want the ticket merged you can set a milestone to something else than "sage-pending"

[kcrisman]

Thanks; in this case I think we are waiting for ​https://github.com/sympy/sympy/pull/8817 to be merged in upstream unstable.

[asmeurer]

I have merged the upstream PR.

[kcrisman]

Awesome, thanks.

[mmezzarobba]

Conflicts with #17493

[git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

​813e19f	14723: fix conflict
​3dccb4e	17493: add _sage_ method to ComplexInfinity
​7ca600a	17493: doctest
​4928fa4	17493: remove unrelated patchfile
​1aff5f2	Merge branch 'develop' into t/17493/bind_sympy_s_complexinfinity
​27e2fc7	17493: add unsigned_infinity conversion to sympy
​a514394	Reviewer patch
​6c74620	Merge branch 'u/kcrisman/ticket/17493' of trac.sagemath.org:sage into t/14723/i14723

[rws]

Merged #14723. Please review fix.

[mmezzarobba]

The following test failure may be related to the changes in this ticket (no time to check right now, sorry).

**********************************************************************
File "src/sage/tests/french_book/recequadiff.py", line 387, in sage.tests.french_book.recequadiff
Failed example:
    rsolve(f, u(n), {u(0):-1,u(1):1})
Exception raised:
    Traceback (most recent call last):
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 488, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 850, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.french_book.recequadiff[107]>", line 1, in <module>
        rsolve(f, u(n), {u(Integer(0)):-Integer(1),u(Integer(1)):Integer(1)})
      File "/home/marc/co/sage/local/lib/python2.7/site-packages/sympy/solvers/recurr.py", line 733, in rsolve
        "'%s(%s+k)' expected, got '%s'" % (y.func, n, h))
    ValueError: 'u(n+k)' expected, got 'u(n + 1)'
**********************************************************************

[rws]

I don't get this error. Please recheck.

[mmezzarobba]

Replying to rws:

I don't get this error. Please recheck.

Apparently I had an older version of your branch. The problem goes away with the last version. Sorry for the noise.

[mmezzarobba]

And now I've compiled a version of sage including all positively reviewed changes, and the test fails again. Do you see any other ticket that may be the cause the problem? I will also launch a brute force git bisect, but it may take a while to complete.

[rws]

Can only be a Sympy problem, or the way Sympy is used. Did you do make to include the patches from #17493 and #14723 into your Sage?

[mmezzarobba]

Replying to rws:

Can only be a Sympy problem, or the way Sympy is used.

Yup, that's why I thought the problem may come from this ticket.

Did you do make to include the patches from #17493 and #14723 into your Sage?

Yes.

[mmezzarobba]

Replying to mmezzarobba:

I will also launch a brute force git bisect, but it may take a while to complete.

No success with that, there are commits where sage fails to build or similar situations that make it a pain.

If I understand correctly, the n in result = h.args[0].match(n + k) (recurr.py:727) somehow ends up being different than the one in appearing in the argument of h, so that the match fails. I suspect there is a conversion from sympy to sage and back going on somewhere, but I have no idea how this all works...

[rws]

Can you attach here a diff against develop of your all-positive branch so that I can try to confirm your result at least?

[mmezzarobba]

Replying to rws:

Can you attach here a diff against develop of your all-positive branch so that I can try to confirm your result at least?

I pushed to u/mmezzarobba/tmp/sympy a branch that contains all the commits I was playing with yesterday (if I remember correctly) plus some, and with which the test still fails. In the meantime I am rebuilding the current trac/develop plus the branch from this ticket...

[mmezzarobba]

Replying to mmezzarobba:

Replying to rws:

Can you attach here a diff against develop of your all-positive branch so that I can try to confirm your result at least?

I pushed to u/mmezzarobba/tmp/sympy a branch that contains all the commits I was playing with yesterday (if I remember correctly) plus some, and with which the test still fails. In the meantime I am rebuilding the current trac/develop plus the branch from this ticket...

...and the test still fails with that build, even after ./sage -i -f sympy ; make build.

[rws]

I don't think I can download your branch, keeps asking me for a password.

[kcrisman]

See this link, though: ​http://git.sagemath.org/sage.git/diff/?id=29e862a147f129c5694f5cbd83338826f9a9e468&h=u%2Fmmezzarobba%2Ftmp%2Fsympy

[mmezzarobba]

Replying to rws:

I don't think I can download your branch, keeps asking me for a password.

I don't know what is going on, but in view of my previous comment, it doesn't matter...

[rws]

OK, it's definitely the integrate patch, i.e., this ticket (not the other).

[rws]

In Sympy master it works fine, maybe there is something with our 01_no-mpmath.patch. I'd rather wait for sympy-0.7.7 which doesn't need it anymore.

[rws]

Summary:

The Sympy patch of this branch contains two hunks, one patches integral.py, the other function.py. The latter one adds a _sage_ method to AppliedUndef but uses a deprecated call method. The added _sage_ method is already there in sympy-1 but gets deleted by a patch in #20185 because it triggers the same error in #20185 as Marc reported here (I was too stupid to rebuild Sympy to see it). So the problem actually exists. However, without the patch this branch does not work.

The first part (the one for integral.py) is no longer necessary because it is in Sympy already.

So what needs to be done is to fix in Sympy and/or Sage the described error. It is investigated in #20185 and its dedicated ticket is #20204, so we depend on that.

[tscrim]

While trying to debug #23496, I noticed that assumptions are lost going to/from Sympy and Sage, which was causing different Sympy Symbols to be created. This may cause some subtle problems at some point if we just work around the problem on #23496 instead of fixing it.

[rws]

All conversion problems were resolved in 8.1.rc0. The problematic integrals in this tickets, including the triple one, work now without any additional code. What remains for this ticket is to add doctests.

[rws]

New commits:

​119f46f

14723: Doctest conversion from SymPy of unevaluated integrals

[tscrim]

LGTM.