Opened 7 years ago
Closed 28 hours ago
#12731 closed defect (fixed)
Disable abs_integrate
Reported by:  roed  Owned by:  burcin 

Priority:  critical  Milestone:  sage8.8 
Component:  calculus  Keywords:  abs_integrate 
Cc:  nbruin, mjo, kcrisman, pbruin, rws, paulmasson, mforets  Merged in:  
Authors:  Ralf Stephan, Frédéric Chapoton  Reviewers:  Dima Pasechnik 
Report Upstream:  N/A  Work issues:  
Branch:  b5c9cc5 (Commits)  Commit:  b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2 
Dependencies:  Stopgaps: 
Description (last modified by )
Maxima's abs_integrate
contributed package gives us lots of nice new integrals, but also really breaks a lot of stuff, apparently. Add to this list as you see fit:
 #13733 Incorrect integral in Maxima
 #11590 Integrating the sgn() function can produce incorrect results
 #14591 cosh integral incorrect because of abs_integrate
 #16643 integrate() infinite loop
 #17183 Yet another abs_integrate trig integral problem
 #17468 Strange integration error/hang with log(abs(sin(x)))
 #17511 Get integral of abs(sin(x)) and abs(cos(x)) right
 #17910
 #23271
 #24117
 This is also blocking update to maxima 5.42, #26625
Old:
Can someone more knowledgeable about using maxima for integration determine what the right warning to show is and when to show it? See #12691 for what a stopgap is.
Change History (49)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
 Priority changed from blocker to major
comment:3 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:4 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:5 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:6 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:7 Changed 4 years ago by
 Cc pbruin rws added
 Description modified (diff)
 Summary changed from Stopgap for #11590 to Disable abs_integrate as default
comment:8 Changed 4 years ago by
 Summary changed from Disable abs_integrate as default to Disable or raise warning with abs_integrate as default
Naturally, if it becomes unnecessary to do so, we can just close this as wontfix. This is the best equivalent to a stopgap I can come up with  or possibly we can raise a warning every time abs_integrate
is used, though this won't stop the hangs and other weird interactions it causes.
comment:9 Changed 4 years ago by
 Description modified (diff)
comment:10 Changed 4 years ago by
Unfortunately, a lot of the most popular such integrals are not computed with sympy (yet?). Not even abs(x)
.
In [1]: from sympy import * In [2]: x=Symbol('x') In [3]: integrate( abs(x), x) Out[3]: Integral(Abs(x), x) In [4]: integrate( abs(x), (x, 0, 1)) Out[4]: Integral(Abs(x), (x, 0, 1))
comment:11 Changed 4 years ago by
Useful comment from author of abs_integrate
:
The assignments extra_definite_integration_methods : [] and extra_integration_methods : [] should render abs_integrate inoperative. Resorting these lists to their defaults will revive abs_integrate. A kill(all) will remove all traces of abs_integratea reload is needed.
So we could conceivably do this in the case that Maxima (or whomever) returns an unevaluated integral, sort of like what we do with solving.
comment:12 followup: ↓ 14 Changed 4 years ago by
could all developers involved in this ticket eventually meditate on Williams comment:
And why are we using it (abs_integrate) if it is so broken  it should require an explicit flag to use at all. Sage is supposed to default to correct answers unless you otherwise explicitly request otherwise
Does that sound reasonable?
comment:13 Changed 4 years ago by
 Milestone changed from sage6.4 to sage6.6
 Priority changed from major to critical
comment:14 in reply to: ↑ 12 Changed 4 years ago by
 Summary changed from Disable or raise warning with abs_integrate as default to Disable abs_integrate
Replying to jakobkroeker:
could all developers involved in this ticket eventually meditate on Williams comment:
And why are we using it (abs_integrate) if it is so broken  it should require an explicit flag to use at all. Sage is supposed to default to correct answers unless you otherwise explicitly request otherwise
Does that sound reasonable?
Yes, and it's the only option because that package causes so much collateral damage, not only in integrate
. Turning it off only triggers a few minor doctest failures, most of them (9) in interfaces/maxima_lib.py
.
comment:15 Changed 4 years ago by
 Branch set to u/rws/disable_abs_integrate
comment:16 Changed 4 years ago by
 Commit set to 4790fc753098c35bbd5d2580b77f77ac511f3fdd
This commit removes the package, fixes/moves doctests. I have put those doctests testing the abs_integrate
package into the wishlist #17910.
If someone is not comfortable doing this "silently", using expression tree walking I could add checks for abs
and sgn
and output ... what? Note, some integrals are now solved by Maxima proper.
New commits:
4790fc7  12731: don't load abs_integrate; move/fix/remove doctests

comment:17 Changed 4 years ago by
 Status changed from new to needs_info
comment:18 Changed 4 years ago by
I think anybody using a computer algebra package for symbolic integration should be aware that results returned might not be correct (or require a different interpretation from what the user would have preferred). However, from the reports listed it seems abs_integrate has particularly numerous issues. It's just not ready for prime time yet. I'd be in favour of ditching it.
Incidentally, all those abs_integrate doctests (and the loading of many of the packages) should probably be in calculus. It's got little to do with the interface. Plus, it used to be as simple as changing the assignment to the maxima interface in the calculus file to swap between the library and the expect interface.
comment:19 Changed 4 years ago by
 Status changed from needs_info to needs_review
comment:20 Changed 4 years ago by
I'm really concerned about removing this completely. Even with all the errors, this also introduces a very useful tool, and it makes Sage look just as bad to be constantly changing what functionality it has. This has been in long enough that some sort of deprecation seems necessary, though I'm not sure myself what either.
See comment:11 for a tip on how to disable/enable it selectively. I have essentially had no Sage development time, nor am likely to in the near future, so I am unable to implement this now. But I think that it would be far better to add some kind of flag or algorithm argument (like with solve()
) so that people who want it can still get it. Especially if it still added a stopgap warning.
Perhaps one could do a walk for relevant input, as you say in comment:16, and suggest using the flag... or one could check for that input and then send the stopgap message?
Again, I'm really sorry I'm unable to help concretely with code at this time (hopefully a little testing).
comment:21 followup: ↓ 23 Changed 4 years ago by
So how do you address the fact that, once the package is used, it has destructive side effects on computations, even if you no longer use it and do completely different things. How will you know that an unrelated bug report is not caused by this? What language do you use to prepare the user that, from now on, there might be more problems ahead?
comment:22 Changed 4 years ago by
I'm really concerned about removing this completely.
what is about a kind of experimental mode in sage ? if enabled by the user, functions like above became available, otherwise they stay out.
Personally, I'm against accepting broken or halfbaked functionality because as rws says, it is too destructive. I'm experiencing this everyday with Singular CAS. People start using new functionality (which is often only experimental ) and then almost everything is broken and cannot be easily fixed. Tests? Ahahaha !! Mathematicians often consider a single example or a couple of examples as as sufficient test...
comment:23 in reply to: ↑ 21 ; followups: ↓ 24 ↓ 25 Changed 4 years ago by
Replying to rws:
So how do you address the fact that, once the package is used, it has destructive side effects on computations, even if you no longer use it and do completely different things. How will you know that an unrelated bug report is not caused by this? What language do you use to prepare the user that, from now on, there might be more problems ahead?
I would rather disable this than have it give wrong answers, but it looks like it should be possible to emit a warning only when abs_integrate is actually used. According to the Maxima documentation, there is a list of extra integration methods that maxima will attempt if the standard integrate
fails:
Option variable: extra_integration_methods Default value: ['signum_int, 'abs_integrate_use_if] The list extra_integration_methods is a list of functions for integration. When integrate is unable to find an antiderivative, Maxima uses the methods in extra_integration_methods to attempt to determine an antiderivative.
If we set this variable to the empty list, then no additional integrations will be attempted by Maxima (by default). But, we can still try them ourselves. So we could,
 Try the standard Maxima
integrate
, and save the result.  Try
signum_int
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.  Try
abs_integrate_use_if
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.
comment:24 in reply to: ↑ 23 Changed 4 years ago by
Replying to mjo:
 Try the standard Maxima
integrate
, and save the result. Try
signum_int
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer. Try
abs_integrate_use_if
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.
Oh, and if integrate
works, we can skip steps 2 and 3.
comment:25 in reply to: ↑ 23 ; followup: ↓ 26 Changed 4 years ago by
Replying to mjo:
If we set this variable to the empty list, then no additional integrations will be attempted by Maxima (by default). But, we can still try them ourselves. So we could,
 Try the standard Maxima
integrate
, and save the result. Try
signum_int
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer. Try
abs_integrate_use_if
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.
That's an excellent find and it allows us to have much finer grained control over what integration methods should be tried. However, I'm not so sure that "printing a warning" is the right solution. There's a problem with stopgap messages in general:
 they just get printed. They can easily get lost if a lot of other output gets printed.
 they only get printed once. We're dealing here with an issue that only applies to certain integrals. We assume you're OK as long as the message is printed. When you run into the message, then the result you're looking at at that moment is suspect. After that one, the message doesn't get printed anymore, so you should consider all subsequent results suspect. However, since the behaviour is much more similar to the premessage situation, it's easy to think that fresh computations in the same session are NOT suspect.
I think it is better to simply NOT try the suspect integration methods upon a normal integral invocation and only resort to them when a special command/flag is given, e.g.:
sage: integrate(abs(sin(x)),x,0,pi) integrate(abs(sin(x)), x, 0, pi) sage: integrate(abs(sin(x)),x,0,pi, algorithm="spaced_out_maxima") 0
or whatever access method we want to use (and whatever result we get from that).
In any case, we also need to clear extra_definite_integration_methods
.
comment:26 in reply to: ↑ 25 ; followup: ↓ 33 Changed 4 years ago by
If we set this variable to the empty list, then no additional integrations will be attempted by Maxima (by default). But, we can still try them ourselves. So we could,
 Try the standard Maxima
integrate
, and save the result. Try
signum_int
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer. Try
abs_integrate_use_if
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.That's an excellent find and it allows us to have much finer grained control over what integration methods should be tried. However, I'm not so sure that "printing a warning" is the right solution. There's a problem with stopgap messages in general:
 they just get printed. They can easily get lost if a lot of other output gets printed.
 they only get printed once. We're dealing here with an issue that only applies to certain integrals. We assume you're OK as long as the message is printed. When you run into the message, then the result you're looking at at that moment is suspect. After that one, the message doesn't get printed anymore, so you should consider all subsequent results suspect. However, since the behaviour is much more similar to the premessage situation, it's easy to think that fresh computations in the same session are NOT suspect.
I think it is better to simply NOT try the suspect integration methods upon a normal integral invocation and only resort to them when a special command/flag is given, e.g.:
sage: integrate(abs(sin(x)),x,0,pi) integrate(abs(sin(x)), x, 0, pi) sage: integrate(abs(sin(x)),x,0,pi, algorithm="spaced_out_maxima") 0or whatever access method we want to use (and whatever result we get from that).
+1
comment:27 Changed 4 years ago by
Is that ticket really needs_review ? The last comments point to a needs_work.
And notice that a needs_review ticket with no author won't suit Volker :P
comment:28 Changed 4 years ago by
comment:29 Changed 4 years ago by
 Status changed from needs_review to needs_work
One failing doctest:
File "src/sage/functions/piecewise.py", line 811, in sage.functions.piecewise.PiecewisePolynomial.integral Failed example: f.integral() Expected: Piecewise defined function with 1 parts, [[(Infinity, +Infinity), x > 1/2*((sgn(x)  1)*e^(2*x)  2*e^x*sgn(x) + sgn(x) + 1)*e^(x)  1]] Got: Piecewise defined function with 1 parts, [[(Infinity, +Infinity), x > integrate(e^(abs(x)), x, x, +Infinity)]]
comment:30 Changed 4 years ago by
Related? This question.
comment:31 Changed 4 years ago by
 Commit changed from 4790fc753098c35bbd5d2580b77f77ac511f3fdd to 1993a28f9ea2dec2cfb255fddee6f01939f69372
comment:32 Changed 4 years ago by
 Milestone changed from sage6.6 to sage6.8
 Status changed from needs_work to needs_review
comment:33 in reply to: ↑ 26 Changed 4 years ago by
Replying to kcrisman:
If we set this variable to the empty list, then no additional integrations will be attempted by Maxima (by default). But, we can still try them ourselves. So we could,
 Try the standard Maxima
integrate
, and save the result. Try
signum_int
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer. Try
abs_integrate_use_if
, and see if the result matches the first one. If it does, ignore it. Otherwise, emit a warning and return the answer.
It won't, because it will always return something like this:
sage: integrate(1/(1 + abs(x+1) + abs(x1)),x) TypeError: unable to make sense of Maxima expression 'if((_SAGE_VAR_x+1)>0,log(12*_SAGE_VAR_x)/2+log(3)2/3,if((_SAGE_VAR_x1)>0,_SAGE_VAR_x/3+log(3)/21/3,log(2*_SAGE_VAR_x+1)/2))' in Sage
I think it is better to simply NOT try the suspect integration methods upon a normal integral invocation and only resort to them when a special command/flag is given, e.g.:
sage: integrate(abs(sin(x)),x,0,pi) integrate(abs(sin(x)), x, 0, pi) sage: integrate(abs(sin(x)),x,0,pi, algorithm="spaced_out_maxima") 0or whatever access method we want to use (and whatever result we get from that).
+1
I would be in favor of this, and would give positive review to changes disabling by default but allowing this. I think it would be pretty easy to implement, in fact.
 Keep
abs_integrate
loading initially inmaxima_lib.py
.  Immediately set the two
methods
to[]
, perhaps right withinit_code.append('nolabels : true')
a few lines later.  Then add an integrator in
src/sage/symbolic/integration/external.py
, something likedef maxima_absint_integrator(expression, v, a=None, b=None): from sage.calculus.calculus import maxima maxima.eval("extra_definite_integration_methods=['abs_defint]") maxima.eval("extra_integration_methods=['signum_int]") # NOT using the if variant if not isinstance(expression, Expression): expression = SR(expression) if a is None: result = maxima.sr_integral(expression,v) else: result = maxima.sr_integral(expression, v, a, b) maxima.eval("extra_definite_integration_methods=[]") maxima.eval("extra_integration_methods=[]") # NOT using the if variant return result._sage_()
 Then in
src/sage/symbolic/integration/integral.py
just addimport sage.symbolic.integration.external as external available_integrators['maxima'] = external.maxima_integrator available_integrators['maxima_absint'] = external.maxima_absint_integrator available_integrators['sympy'] = external.sympy_integrator available_integrators['mathematica_free'] = external.mma_free_integrator available_integrators['fricas'] = external.fricas_integrator
and port doctests, with a humongous warning not to use this unless you are willing to check answers numerically or something.
That could, in principle, be a separate ticket, but I would want them to depend upon each other. Also, that wouldn't remove the necessity of #17910.
By the way, it's interesting that http://sourceforge.net/p/maxima/bugs/2242/#8ddc suggests that adding 'integrate
to the methods lists could actually do at least some of these integrals...
comment:34 Changed 4 years ago by
 Status changed from needs_review to needs_work
Never remove doctests, mark them as # known bug (Trac #17910)
.
comment:35 Changed 4 years ago by
 Description modified (diff)
comment:36 Changed 3 years ago by
 Cc paulmasson added
comment:37 Changed 18 months ago by
 Description modified (diff)
comment:38 Changed 18 months ago by
 Cc mforets added
comment:39 Changed 4 weeks ago by
 Description modified (diff)
comment:40 Changed 4 weeks ago by
 Branch changed from u/rws/disable_abs_integrate to public/disable_abs_integrate
 Commit changed from 1993a28f9ea2dec2cfb255fddee6f01939f69372 to 209b2c86c77c9069b38f436331d683720a2faeec
New commits:
209b2c8  Merge branch 'u/rws/disable_abs_integrate' in 8.8.b3

comment:41 Changed 4 weeks ago by
 Branch changed from public/disable_abs_integrate to u/rws/disable_abs_integrate
 Commit changed from 209b2c86c77c9069b38f436331d683720a2faeec to 1993a28f9ea2dec2cfb255fddee6f01939f69372
my merge was incorrect, so undone
comment:42 Changed 8 days ago by
 Keywords abs_integrate added
comment:43 Changed 8 days ago by
 Branch changed from u/rws/disable_abs_integrate to u/chapoton/12731
 Commit changed from 1993a28f9ea2dec2cfb255fddee6f01939f69372 to b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2
 Milestone changed from sage6.8 to sage8.8
 Status changed from needs_work to needs_review
comment:44 Changed 8 days ago by
Thanks  I wish there was a better fix than disabling. Can you briefly comment on
 The removal of the radexpand example
 the
known bug
ones
I'm sure they are quite logical but a quick glance fails to reveal it, as opposed to where you put the unevaluated integral in to replace the abs_integrate
behavior.
Or maybe this was already present behavior and you really did just refresh this, in which case no worries. It seems like comment:33 was the last substantive part of the discussion and memory fails.
comment:45 Changed 8 days ago by
Tagging by known bug
allows to remember that one could hope for the answer displayed. I think Jeroen suggested to keep the doctest in this way, rather than remove them.
And the radexpand
example no longer works without abs_integrate
, just returning unevaluated.
comment:46 Changed 7 days ago by
bot is morally green, please review
comment:47 Changed 4 days ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
looks good to me. Please add yourself as authors/reviewers...
comment:48 Changed 4 days ago by
comment:49 Changed 28 hours ago by
 Branch changed from u/chapoton/12731 to b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2
 Resolution set to fixed
 Status changed from positive_review to closed
I'm not really sure that this is appropriate for a stopgap. I mean, unless we put in a catch for the specific form like
integrate(x * sgn(x^2  1/4), x, 1, 0)
, we would have to put in a stopgap for the entire integration, which is ridiculous.Unless we checked for the signum function with nonlinear operands in it sometime... but that's more than I have time to parse now.