Opened 8 years ago

Closed 3 years ago

#14723 closed task (fixed)

Doctest conversion from SymPy of unevaluated integrals

Reported by: eviatarbach Owned by: burcin
Priority: major Milestone: sage-8.2
Component: calculus Keywords: sympy, integrate
Cc: kcrisman, asmeurer, was Merged in:
Authors: Ralf Stephan Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 119f46f (Commits) Commit: 119f46f5119e535c53daaeb2404281714b348f6d
Dependencies: #20204 Stopgaps:

Description (last modified by ppurka)

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)

Attachments (1)

14723.patch (1.2 KB) - added by eviatarbach 8 years ago.

Download all attachments as: .zip

Change History (92)

Changed 8 years ago by eviatarbach

comment:1 Changed 8 years ago by eviatarbach

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

comment:2 Changed 8 years ago by kcrisman

  • Cc kcrisman added

comment:3 Changed 8 years ago by eviatarbach

  • Status changed from new to needs_review

comment:4 Changed 8 years ago by eviatarbach

  • Status changed from needs_review to needs_work

comment:5 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:6 Changed 7 years ago by eviatarbach

  • Cc asmeurer added

comment:7 Changed 7 years ago by asmeurer

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

comment:8 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:9 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:10 Changed 7 years ago by kcrisman

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

comment:11 Changed 7 years ago by ppurka

  • Cc was added
  • Description modified (diff)

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:13 Changed 6 years ago by rws

  • Authors set to Eviatar Bach
  • Keywords sympy integrate added
  • Work issues set to handle definite integrals too

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)

comment:14 Changed 6 years ago by 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

Last edited 6 years ago by rws (previous) (diff)

comment:15 Changed 6 years ago by rws

  • Report Upstream changed from N/A to Reported upstream. Developers acknowledge bug.
  • Work issues changed from handle definite integrals too to fix in sympy

comment:16 follow-up: Changed 6 years ago by 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.

comment:17 Changed 6 years ago by rws

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Completely fixed; Fix reported upstream

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):

comment:18 follow-up: Changed 6 years ago by 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?

comment:19 in reply to: ↑ 18 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by rws

  • Branch set to u/rws/i14723

comment:21 Changed 6 years ago by rws

  • Authors changed from Eviatar Bach to Eviatar Bach, Ralf Stephan
  • Commit set to 8c345ce80f75b1936218b60b34ebc070ce1f9b55
  • Status changed from needs_work to needs_review

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


New commits:

8c345ce14723: implement Integrate._sage_ in intalled sympy

comment:22 Changed 6 years ago by git

  • Commit changed from 8c345ce80f75b1936218b60b34ebc070ce1f9b55 to cf6f7b1b64cdde5ec0fb4a33fff3e58db6ac59d6

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

cf6f7b114723: change patchlevel too

comment:23 follow-up: Changed 6 years ago by rws

  • Status changed from needs_review to needs_work

Some issues surfaced, see Sympy pull request.

comment:24 in reply to: ↑ 16 Changed 6 years ago by rws

  • Dependencies set to #2787

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.

comment:25 Changed 6 years ago by 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.

comment:26 Changed 6 years ago by rws

  • Dependencies #2787 deleted

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

comment:27 in reply to: ↑ 23 ; follow-up: Changed 6 years ago by 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.

comment:28 in reply to: ↑ 27 Changed 6 years ago by 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.

comment:29 follow-up: Changed 6 years ago by 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.

Last edited 6 years ago by rws (previous) (diff)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 6 years ago by 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.

comment:31 Changed 6 years ago by git

  • Commit changed from cf6f7b1b64cdde5ec0fb4a33fff3e58db6ac59d6 to 099b9f6f1a67a865978bb84fd6c3a2ce29427aaf

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

099b9f614723: handle multiple integrals from SymPy too

comment:32 in reply to: ↑ 30 ; follow-up: Changed 6 years ago by 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.

comment:33 in reply to: ↑ 32 Changed 6 years ago by 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.

comment:34 Changed 6 years ago by 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.

comment:35 Changed 6 years ago by rws

  • Status changed from needs_work to needs_review

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.

comment:36 Changed 6 years ago by 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.

comment:37 Changed 6 years ago by 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).

comment:38 Changed 6 years ago by git

  • Commit changed from 099b9f6f1a67a865978bb84fd6c3a2ce29427aaf to af20fadfbbbbbff27f6cd7779ed79bf1f494551f

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

af20fad14723: better testing, patchlevel

comment:39 Changed 6 years ago by rws

  • Milestone changed from sage-6.4 to sage-pending

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

comment:40 Changed 6 years ago by kcrisman

  • Reviewers set to Ralf Stephan, Karl-Dieter Crisman
  • Status changed from needs_review to positive_review

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!

comment:41 Changed 6 years ago by 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.

comment:42 Changed 6 years ago by git

  • Commit changed from af20fadfbbbbbff27f6cd7779ed79bf1f494551f to 4c450f8bb5ddf4253a3db347f865a7a115abfb8f
  • 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:

4c450f814723: the crux of the biscuit is the apostrophe

comment:43 Changed 6 years ago by kcrisman

  • Status changed from needs_review to positive_review

That was a good catch.

comment:44 follow-up: Changed 6 years ago by 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.

comment:45 in reply to: ↑ 44 ; follow-up: Changed 6 years ago by 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.

comment:46 in reply to: ↑ 45 Changed 6 years ago by 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).

comment:47 Changed 6 years ago by asmeurer

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

comment:48 Changed 6 years ago by rws

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

comment:49 Changed 6 years ago by git

  • Commit changed from 4c450f8bb5ddf4253a3db347f865a7a115abfb8f to dd4f7ffae3eb5a98734430f5c94347d9c785989b
  • 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:

dd4f7ff14723: accept undefined functions from SymPy

comment:50 follow-up: Changed 6 years ago by 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).

comment:51 in reply to: ↑ 50 Changed 6 years ago by rws

Replying to kcrisman:

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

Yes, it's now added.

comment:52 Changed 6 years ago by 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.

comment:53 Changed 6 years ago by git

  • Commit changed from dd4f7ffae3eb5a98734430f5c94347d9c785989b to e2f0713047e65e20969361725e3c29090379eb0d

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

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

comment:54 Changed 6 years ago by kcrisman

  • Status changed from needs_review to positive_review
  • Work issues fix in sympy deleted

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.

comment:55 Changed 6 years ago by 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?)

comment:56 Changed 6 years ago by git

  • Commit changed from e2f0713047e65e20969361725e3c29090379eb0d to 880a6177ed7f1b260c19e2b3b92eef0f0f516daf
  • 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:

880a61714723: fix doctest

comment:57 Changed 6 years ago by kcrisman

  • Status changed from needs_review to positive_review

Thanks for all the work here.

comment:58 Changed 6 years ago by rws

Thanks for the review.

comment:59 Changed 6 years ago by vbraun

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

comment:60 Changed 6 years ago by kcrisman

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

comment:61 Changed 6 years ago by asmeurer

I have merged the upstream PR.

comment:62 Changed 6 years ago by kcrisman

  • Milestone changed from sage-pending to sage-6.5

Awesome, thanks.

comment:63 Changed 6 years ago by mmezzarobba

Conflicts with #17493

comment:64 Changed 6 years ago by git

  • Commit changed from 880a6177ed7f1b260c19e2b3b92eef0f0f516daf to 6c746203fff98cdafe01b4a6cd95e7c2880366c4
  • 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:

813e19f14723: fix conflict
3dccb4e17493: add _sage_ method to ComplexInfinity
7ca600a17493: doctest
4928fa417493: remove unrelated patchfile
1aff5f2Merge branch 'develop' into t/17493/bind_sympy_s_complexinfinity
27e2fc717493: add unsigned_infinity conversion to sympy
a514394Reviewer patch
6c74620Merge branch 'u/kcrisman/ticket/17493' of trac.sagemath.org:sage into t/14723/i14723

comment:65 Changed 6 years ago by rws

Merged #14723. Please review fix.

comment:66 Changed 6 years ago by 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)'
**********************************************************************

comment:67 follow-up: Changed 6 years ago by rws

I don't get this error. Please recheck.

comment:68 in reply to: ↑ 67 Changed 6 years ago by mmezzarobba

  • Status changed from needs_review to positive_review

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.

comment:69 follow-up: Changed 6 years ago by 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.

comment:70 follow-up: Changed 6 years ago by 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?

comment:71 in reply to: ↑ 70 Changed 6 years ago by 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.

comment:72 in reply to: ↑ 69 Changed 6 years ago by 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...

comment:73 follow-up: Changed 6 years ago by 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?

comment:74 in reply to: ↑ 73 ; follow-up: Changed 6 years ago by 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...

comment:75 in reply to: ↑ 74 Changed 6 years ago by 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.

comment:76 follow-up: Changed 6 years ago by rws

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

comment:78 in reply to: ↑ 76 Changed 6 years ago by 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...

comment:79 Changed 6 years ago by mmezzarobba

  • Status changed from positive_review to needs_work

comment:80 Changed 6 years ago by rws

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

comment:81 Changed 6 years ago by 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.

comment:82 Changed 6 years ago by rws

  • Milestone changed from sage-6.5 to sage-pending

comment:83 Changed 5 years ago by rws

  • Dependencies set to #20815

comment:84 Changed 5 years ago by rws

  • Dependencies changed from #20815 to #20185

comment:85 Changed 4 years ago by rws

  • Dependencies changed from #20185 to #20204
  • Report Upstream changed from Completely fixed; Fix reported upstream to None of the above - read trac for reasoning.

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.

comment:86 Changed 3 years ago by 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.

comment:87 Changed 3 years ago by rws

  • Authors Eviatar Bach, Ralf Stephan deleted
  • Branch u/rws/i14723 deleted
  • Commit 6c746203fff98cdafe01b4a6cd95e7c2880366c4 deleted
  • Milestone changed from sage-pending to sage-8.2
  • Report Upstream changed from None of the above - read trac for reasoning. to N/A
  • Reviewers Ralf Stephan, Karl-Dieter Crisman deleted

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.

comment:88 Changed 3 years ago by rws

  • Branch set to u/rws/14723-1

comment:89 Changed 3 years ago by rws

  • Authors set to Ralf Stephan
  • Commit set to 119f46f5119e535c53daaeb2404281714b348f6d
  • Status changed from needs_work to needs_review
  • Summary changed from Error when SymPy can't evaluate an integral to Doctest conversion from SymPy of unevaluated integrals
  • Type changed from defect to task

New commits:

119f46f14723: Doctest conversion from SymPy of unevaluated integrals

comment:90 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:91 Changed 3 years ago by vbraun

  • Branch changed from u/rws/14723-1 to 119f46f5119e535c53daaeb2404281714b348f6d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.