sagemath / sage

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

Fail to integrate sqrt(x^2)/x #25119

Closed rwst closed 4 years ago

rwst commented 6 years ago
sage: integrate(sqrt(x^2)/x,x)
...
RuntimeError: ECL says: Error executing code in Maxima: expt: undefined: 0 to a negative exponent.

sage: integrate(sqrt(x^2)/x,x,algorithm='fricas')
sqrt(x^2)
sage: integrate(sqrt(x^2)/x,x,algorithm='giac')
x*sign(x)
sage: integrate(sqrt(x^2)/x,x,algorithm='sympy')
sqrt(x^2)

See Maxima bug 3657.

Upstream: Reported upstream. No feedback yet.

CC: @slel @kcrisman

Component: calculus

Keywords: integral

Author: Frédéric Chapoton

Branch/Commit: 744d626

Reviewer: Karl-Dieter Crisman

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

fchapoton commented 4 years ago

Author: Frédéric Chapoton

fchapoton commented 4 years ago

Branch: u/chapoton/25119

fchapoton commented 4 years ago

Changed keywords from none to integral

fchapoton commented 4 years ago
comment:1

Here is a fix (bandaid). One should report upstream to maxima.


New commits:

744d626fix some details in integration, make one more integral work
fchapoton commented 4 years ago

Commit: 744d626

fchapoton commented 4 years ago
comment:2

bots are morally green, please review

kcrisman commented 4 years ago
comment:3

Thanks, Frédéric. Can I ask whether the changes other than the new doctest and the addition of RunTimeError are nontrivial? I don't think so, but there were a lot of prettification changes.

kcrisman commented 4 years ago
comment:4

In vanilla Maxima:

(%i4) domain:complex;
(%o4)                               complex
(%i5) integrate(sqrt(x^2)/x,x);

expt: undefined: 0 to a negative exponent.
 -- an error. To debug this try: debugmode(true);
kcrisman commented 4 years ago
comment:5

However, before giving positive review, I'd suppose we'd want a way to check that this one was fixed - maybe # known bug below it where we require algorithm='maxima'?

kcrisman commented 4 years ago

Description changed:

--- 
+++ 
@@ -12,3 +12,5 @@
 sqrt(x^2)

+See Maxima bug 3657. +

kcrisman commented 4 years ago

Upstream: Reported upstream. No feedback yet.

fchapoton commented 4 years ago
comment:7

All the other changes are purely white space removal or addition, for the sake of flake8 conmpliance.

I guess one could a "known bug" doctest, yes.

kcrisman commented 4 years ago
comment:8

Ah yes. I haven't used that, personally, but I'm sure it complains a lot. Unfortunately, it just makes tickets harder to review at times. I won't raise this on -devel because I know how annoyingly much extra work it would be, but having two different commits for that sort of thing is helpful to reviewers.

fchapoton commented 4 years ago
comment:9

Do you want the "known bug" doctest ? This does not seem to be really necessary to me. We are not responsible for maxima bugs.

kcrisman commented 4 years ago

Reviewer: Karl-Dieter Crisman

kcrisman commented 4 years ago
comment:10

It would be nice, because we typically do this in other cases. But I guess since in this case it is an actual exception raised, as opposed to a wrong result we had to work around, it is not necessary.

But if Volker complains about failing doctests, I am trusting your morally green patchbot :-) Just upgraded OS (still several versions behind) and so won't be building new Sage for a little while until I have time to check that command line tools are working properly.

vbraun commented 4 years ago

Changed branch from u/chapoton/25119 to 744d626