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

Add doctest for elliptic integrals of the second kind #26563

Closed
EmmanuelCharpentier mannequin opened this issue Oct 26, 2018 · 20 comments
Closed

Add doctest for elliptic integrals of the second kind #26563

EmmanuelCharpentier mannequin opened this issue Oct 26, 2018 · 20 comments

Comments

@EmmanuelCharpentier
Copy link
Mannequin

EmmanuelCharpentier mannequin commented Oct 26, 2018

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

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

@EmmanuelCharpentier EmmanuelCharpentier mannequin added this to the sage-8.5 milestone Oct 26, 2018
@mantepse
Copy link
Collaborator

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

@EmmanuelCharpentier

This comment has been minimized.

@fchapoton
Copy link
Contributor

Changed keywords from integration to integration, abs_integrate

@fchapoton
Copy link
Contributor

comment:5

fixed by #27958, that needs review

@orlitzky
Copy link
Contributor

New commits:

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

@orlitzky
Copy link
Contributor

Author: Michael Orlitzky

@orlitzky
Copy link
Contributor

Branch: u/mjo/ticket/26563

@orlitzky
Copy link
Contributor

Commit: 117fb42

@fchapoton
Copy link
Contributor

comment:7

red branch => needs work

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2021

Changed commit from 117fb42 to 8a052c5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 25, 2021

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
Copy link
Contributor

comment:9

Rebased onto develop.

@tscrim
Copy link
Collaborator

tscrim commented Jan 26, 2021

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jan 26, 2021

comment:10

LGTM.

@mkoeppe mkoeppe changed the title Sage screws up the integration of some functions. Add doctest for elliptic integrals of the second kind Jan 26, 2021
@fchapoton
Copy link
Contributor

comment:12

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

@tscrim
Copy link
Collaborator

tscrim commented Jan 27, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

vbraun commented Feb 20, 2021

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

@vbraun vbraun closed this as completed in 7733263 Feb 20, 2021
@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.3 Mar 14, 2021
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

6 participants