sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.37k stars 462 forks source link

Add doctest for elliptic integrals of the second kind #26563

Closed 7822f248-ba56-45f1-ab3d-4de7482bdf9f closed 3 years ago

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 5 years ago

Inspiration this ask.sagemath question.Edit: this one seems to be another instance of the same problem.

A lot of tickets describe indefinite integral bugs attributable to maxima, most notably its abs_integrate package ; see #12731 for a sample of the latter, and the list of integration tickets for somother infamous examples...

However, Sage, which is becoming a mature system, seems to have become able to screw up things by itself on its own, without any external help. Case in point :

sage: elliptic_e(x,1/2).diff(x)
sqrt(-1/2*sin(x)^2 + 1)
sage: maxima.integrate(sqrt(1-m*sin(x)^2),x).sage()
integrate(sqrt(-m*sin(x)^2 + 1), x)
sage: integrate(sqrt(1-m*sin(x)^2),x)
1/4*m*x - 1/8*m*sin(2*x)

which is wrong, wrong, wrong...

CC: @mforets @fchapoton

Component: symbolics

Keywords: integration, abs_integrate

Author: Michael Orlitzky

Branch/Commit: 8a052c5

Reviewer: Travis Scrimshaw

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

mantepse commented 5 years ago
comment:1

I think that this actually is due to abs_integrate:

(%i1) integrate(sqrt(1-m*sin(x)^2),x);
                           /
                           [               2
(%o1)                      I sqrt(1 - m sin (x)) dx
                           ]
                           /
(%i2) load(abs_integrate);
ARRSTORE: use_fast_arrays=false; allocate a new property hash table for $INTABLE2
(%o2) sage-develop/local/share/maxima/5.41.0/share/contrib/integration/abs_integrate.mac
(%i3) integrate(sqrt(1-m*sin(x)^2),x);
                    2 m sin(2 x) false - m sin(2 x) + 2 m x
(%o3)               ---------------------------------------
                                       8

(although it's quite interesting what sage does with the result...)

7822f248-ba56-45f1-ab3d-4de7482bdf9f commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-Inspiration this [ask.sagemath](https://ask.sagemath.org/question/44077/indefinite-integral-is-incorrect/) question.
+Inspiration this [ask.sagemath](https://ask.sagemath.org/question/44077/indefinite-integral-is-incorrect/) question.**Edit:** [this one](https://ask.sagemath.org/question/45245/why-gives-a-wrong-result/) seems to be another instance of the same problem.

 A lot of tickets describe indefinite integral bugs attributable to `maxima`, most notably its `abs_integrate` package ; see #12731 for a sample of the latter, and the [list](https://trac.sagemath.org/wiki/symbolics#Integrationtickets) of integration tickets for somother infamous examples...
fchapoton commented 5 years ago

Changed keywords from integration to integration, abs_integrate

fchapoton commented 5 years ago
comment:5

fixed by #27958, that needs review

orlitzky commented 3 years ago

New commits:

117fb42Trac #26563: check the fundamental theorem of calculus for elliptic_e().
orlitzky commented 3 years ago

Author: Michael Orlitzky

orlitzky commented 3 years ago

Branch: u/mjo/ticket/26563

orlitzky commented 3 years ago

Commit: 117fb42

fchapoton commented 3 years ago
comment:7

red branch => needs work

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 117fb42 to 8a052c5

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

8a052c5Trac #26563: check the fundamental theorem of calculus for elliptic_e().
orlitzky commented 3 years ago
comment:9

Rebased onto develop.

tscrim commented 3 years ago

Reviewer: Travis Scrimshaw

tscrim commented 3 years ago
comment:10

LGTM.

fchapoton commented 3 years ago
comment:12

What about the time it takes ? Do we really need this doctest ?

tscrim commented 3 years ago
comment:13

It takes about 15s on my computer. While it is not very long, it still is a bit long. It is important to have a regression test, which is why I gave it a positive review. Yet, I do see your point about not increasing the (long) test time too much. It suggests a change in the overall testing framework in Sage with respect to the symbolics. Perhaps we need to add the tests explicitly to the installation check? Although I lean towards keeping this at least for now to demonstrate where we have had issues. Frédéric, your thoughts?

orlitzky commented 3 years ago
comment:14

It's slow but it's also kind of a cool example. Personally I'd prefer to delete ten other symbol-salad integration tests to make up for the time this one takes =)

(IMO anything tested by the upstream test suite is a waste of time to duplicate within the sage library, since users have the option to run those test suites as well.)

fchapoton commented 3 years ago
comment:15

Well, my point is that if everybody happily adds new doctests, only a few people care about how long it takes, and nobody at all cares about the ever-increasing time required to doctest sage. May I recall that a long doctest should rather not take more than 5s ?

https://doc.sagemath.org/html/en/developer/doctesting.html#run-long-doctests

Maybe specifying the algorithm that works would shorten the test time by not trying the failing algorithms ?

orlitzky commented 3 years ago
comment:16

In this case the point of the test is that maxima "fails to integrate it incorrectly," after which sympy produces the correct result. When maxima returns a wrong answer (rather than giving up), sympy is never tried and we just return the wrong thing. Maxima gives up quickly, but the rest of the time spent in the doctest is just sympy performing the integral and getting the right answer.

I'm fine if we want to leave this doctest out. I was just trying to help close out an old ticket by providing a test that shows that it's fixed.

(And in general, if you ever want to propose that we go back and delete all of the "fixed in a new version of $package" doctests, I'd be all for it. I already run the test suites for maxima, pari, flint, etc. and don't need to test for those bugs all over again when I build sage.)

vbraun commented 3 years ago

Changed branch from u/mjo/ticket/26563 to 8a052c5