Opened 3 years ago

Closed 7 months ago

Last modified 6 months ago

#26563 closed defect (fixed)

Add doctest for elliptic integrals of the second kind

Reported by: charpent Owned by:
Priority: major Milestone: sage-9.3
Component: symbolics Keywords: integration, abs_integrate
Cc: mforets, chapoton Merged in:
Authors: Michael Orlitzky Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8a052c5 (Commits, GitHub, GitLab) Commit: 8a052c5256cbc7ae39af6bd2fd9bd4f95c076b13
Dependencies: Stopgaps:

Status badges

Description (last modified by charpent)

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)
  • Maxima can't solve the reverse problem, but will honestly report its failure :
    sage: maxima.integrate(sqrt(1-m*sin(x)^2),x).sage()
    integrate(sqrt(-m*sin(x)^2 + 1), x)
  • Sage will lie ;-) :
    sage: integrate(sqrt(1-m*sin(x)^2),x)
    1/4*m*x - 1/8*m*sin(2*x)

which is wrong, wrong, wrong...

Change History (18)

comment:1 Changed 3 years ago by mantepse

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

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

comment:2 Changed 3 years ago by mforets

  • Cc mforets added

comment:3 Changed 3 years ago by charpent

  • Description modified (diff)

comment:4 Changed 2 years ago by chapoton

  • Keywords abs_integrate added

comment:5 Changed 2 years ago by chapoton

fixed by #27958, that needs review

comment:6 Changed 9 months ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/26563
  • Cc chapoton added
  • Commit set to 117fb428cb898d479383a381fff302ba4848a43f
  • Status changed from new to needs_review

New commits:

117fb42Trac #26563: check the fundamental theorem of calculus for elliptic_e().

comment:7 Changed 8 months ago by chapoton

  • Milestone changed from sage-8.5 to sage-9.4
  • Status changed from needs_review to needs_work

red branch => needs work

comment:8 Changed 8 months ago by git

  • Commit changed from 117fb428cb898d479383a381fff302ba4848a43f to 8a052c5256cbc7ae39af6bd2fd9bd4f95c076b13

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().

comment:9 Changed 8 months ago by mjo

  • Status changed from needs_work to needs_review

Rebased onto develop.

comment:10 Changed 8 months ago by tscrim

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


comment:11 Changed 8 months ago by mkoeppe

  • Summary changed from Sage screws up the integration of some functions. to Add doctest for elliptic integrals of the second kind

comment:12 Changed 8 months ago by chapoton

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

comment:13 Changed 8 months ago by tscrim

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?

comment:14 Changed 8 months ago by mjo

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

comment:15 Changed 8 months ago by chapoton

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 ?

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

comment:16 Changed 8 months ago by mjo

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

comment:17 Changed 7 months ago by vbraun

  • Branch changed from u/mjo/ticket/26563 to 8a052c5256cbc7ae39af6bd2fd9bd4f95c076b13
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 6 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.3
Note: See TracTickets for help on using tickets.