Opened 8 years ago

Closed 7 months ago

Last modified 5 months ago

#12731 closed defect (fixed)

Disable abs_integrate

Reported by: roed Owned by: burcin
Priority: critical Milestone: sage-8.9
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:
Dependencies: Stopgaps:

Description (last modified by arojas)

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

comment:1 Changed 8 years ago by kcrisman

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.

comment:2 Changed 8 years ago by jdemeyer

  • Priority changed from blocker to major

comment:3 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:4 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:5 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:6 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:7 Changed 5 years ago by kcrisman

  • Cc pbruin rws added
  • Description modified (diff)
  • Summary changed from Stopgap for #11590 to Disable abs_integrate as default

comment:8 Changed 5 years ago by kcrisman

  • 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 5 years ago by kcrisman

  • Description modified (diff)

comment:10 Changed 5 years ago by kcrisman

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 5 years ago by kcrisman

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_integrate--a 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 follow-up: Changed 5 years ago by jakobkroeker

@kcrisman could you 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

Version 0, edited 5 years ago by jakobkroeker (next)

comment:13 Changed 5 years ago by rws

  • Milestone changed from sage-6.4 to sage-6.6
  • Priority changed from major to critical

comment:14 in reply to: ↑ 12 Changed 5 years ago by rws

  • 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 5 years ago by rws

  • Branch set to u/rws/disable_abs_integrate

comment:16 Changed 5 years ago by rws

  • 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:

4790fc712731: don't load abs_integrate; move/fix/remove doctests

comment:17 Changed 5 years ago by rws

  • Status changed from new to needs_info

comment:18 Changed 5 years ago by nbruin

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 5 years ago by rws

  • Status changed from needs_info to needs_review

comment:20 Changed 5 years ago by kcrisman

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 follow-up: Changed 5 years ago by 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?

comment:22 Changed 5 years ago by jakobkroeker

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 half-baked 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 ; follow-ups: Changed 5 years ago by mjo

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,

  1. Try the standard Maxima integrate, and save the result.
  2. 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.
  3. 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 5 years ago by mjo

Replying to mjo:

  1. Try the standard Maxima integrate, and save the result.
  2. 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.
  3. 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 ; follow-up: Changed 5 years ago by nbruin

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,

  1. Try the standard Maxima integrate, and save the result.
  2. 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.
  3. 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 pre-message 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 ; follow-up: Changed 5 years ago by 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,

  1. Try the standard Maxima integrate, and save the result.
  2. 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.
  3. 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 pre-message 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).

+1

comment:27 Changed 5 years ago by Snark

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 5 years ago by rws

  • Authors set to Ralf Stephan

comment:29 Changed 5 years ago by chapoton

  • 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 5 years ago by kcrisman

comment:31 Changed 4 years ago by git

  • Commit changed from 4790fc753098c35bbd5d2580b77f77ac511f3fdd to 1993a28f9ea2dec2cfb255fddee6f01939f69372

Branch pushed to git repo; I updated commit sha1. New commits:

2ea38b1Merge branch 'develop' into t/12731/disable_abs_integrate
1993a2812731: remove doctest dependent on abs_integrate

comment:32 Changed 4 years ago by rws

  • Milestone changed from sage-6.6 to sage-6.8
  • Status changed from needs_work to needs_review

comment:33 in reply to: ↑ 26 Changed 4 years ago by kcrisman

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,

  1. Try the standard Maxima integrate, and save the result.
  2. 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.
  3. 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(x-1)),x)
TypeError: unable to make sense of Maxima expression 'if(-(_SAGE_VAR_x+1)>0,-log(1-2*_SAGE_VAR_x)/2+log(3)-2/3,if(-(_SAGE_VAR_x-1)>0,_SAGE_VAR_x/3+log(3)/2-1/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")
0

or 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 in maxima_lib.py.
  • Immediately set the two methods to [], perhaps right with init_code.append('nolabels : true') a few lines later.
  • Then add an integrator in src/sage/symbolic/integration/external.py, something like
    def 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 add
    import 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 jdemeyer

  • Status changed from needs_review to needs_work

Never remove doctests, mark them as # known bug (Trac #17910).

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:35 Changed 4 years ago by slelievre

  • Description modified (diff)

comment:36 Changed 3 years ago by paulmasson

  • Cc paulmasson added

comment:37 Changed 2 years ago by rws

  • Description modified (diff)

comment:38 Changed 2 years ago by mforets

  • Cc mforets added

comment:39 Changed 8 months ago by arojas

  • Description modified (diff)

comment:40 Changed 8 months ago by chapoton

  • Branch changed from u/rws/disable_abs_integrate to public/disable_abs_integrate
  • Commit changed from 1993a28f9ea2dec2cfb255fddee6f01939f69372 to 209b2c86c77c9069b38f436331d683720a2faeec

New commits:

209b2c8Merge branch 'u/rws/disable_abs_integrate' in 8.8.b3

comment:41 Changed 8 months ago by chapoton

  • 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 7 months ago by chapoton

  • Keywords abs_integrate added

comment:43 Changed 7 months ago by chapoton

  • Branch changed from u/rws/disable_abs_integrate to u/chapoton/12731
  • Commit changed from 1993a28f9ea2dec2cfb255fddee6f01939f69372 to b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2
  • Milestone changed from sage-6.8 to sage-8.8
  • Status changed from needs_work to needs_review

Here is a fresh new branch


New commits:

b5c9cc5disable abs_integrate buggy module of maxima

comment:44 Changed 7 months ago by kcrisman

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 7 months ago by chapoton

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 months ago by chapoton

bot is morally green, please review

comment:47 Changed 7 months ago by dimpase

  • 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 7 months ago by chapoton

  • Authors changed from Ralf Stephan to Ralf Stephan, Frédéric Chapoton

comment:49 Changed 7 months ago by vbraun

  • Branch changed from u/chapoton/12731 to b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:50 Changed 6 months ago by kcrisman

  • Commit b5c9cc58cdda15c3d33e5a47ed953bd17989a6c2 deleted

At least according to github, the following stuff in sage/maxima_lib.py was not removed (probably because it still passes doctests as Maxima proper improved in the meantime):

        Make sure the abs_integrate package is being used,
        :trac:`11483`. The following are examples from the Maxima
        abs_integrate documentation::
            sage: integrate(abs(x), x)
            1/2*x*abs(x)

comment:51 Changed 5 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Not in Sage 8.8. Let's please to try keep tickets' milestones related to the release in which we actually intend to include them, and in particular the release in which they were actually included, especially when closing tickets.

Note: See TracTickets for help on using tickets.