Perform safe simplifications in Expression.simplify()

[orlitzky]

This is part 1 (of n, n >= 1) of an attempt to make simplification safer.

Right now, simplify() doesn't attempt anything drastic: it sends the expression to Maxima and back. So if you want a non-trivial simplification, you have to use something else.

That something else is full_simplify(), unless you want to write your own simplification function. But full_simplify() has a problem: the evil radcan.

1. inconsistency in simplify_radical #3520 - inconsistency in simplify_radical
2. invalid simplification of complex logarithm #12322 - invalid simplification of complex logarithm
3. AskSage 767 - simplification errors in simple expressions

And a simple example that logix was nice enough to dig out of my code on IRC:

sage: f = sqrt((x+1)^2)
sage: f.full_simplify()
x + 1
sage: f(x = -5)
4
sage: f.full_simplify()(x = -5)
-4

The goal is to make simplification safer, through some combination of,

• Making simplify() more useful.
• Making it obvious that full_simplify() can do some, uh, unintuitive things.

Unless there are objections, I see no reason not to make simplify() perform all "safe" simplifications; that is, simplifications for which simplify(expr) == expr with some certainty.

Component: symbolics

Reviewer: Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan

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

[orlitzky]

Author: Michael Orlitzky

[roed314]

comment:2

I'm happy with the overall plan, but the upgrade to Maxima 5.26 (#10682, merged in 5.0.beta8) produces a conflict. With your patch, we have the following doctests from sage.calculus.wester.

sage: # (YES) Assuming Re(x)>0, Re(y)>0, deduce x^(1/n)*y^(1/n)-(x*y)^(1/n)=0.
sage: # Maxima 5.26 has different behaviours depending on the current
sage: # domain.
sage: # To stick with the behaviour of previous versions, the domain is set
sage: # to 'real' in the following.
sage: # See Trac #10682 for further details.
sage: n = var('n')
sage: f = x^(1/n)*y^(1/n)-(x*y)^(1/n)
sage: assume(real(x) > 0, real(y) > 0)
sage: f.simplify()
0
sage: maxima = sage.calculus.calculus.maxima
sage: maxima.set('domain', 'real') # set domain to real
sage: f.simplify()
0
sage: maxima.set('domain', 'complex') # set domain back to its default value
sage: forget()

Before your patch, the first f.simplify() didn't do much:

sage: f = x^(1/n)*y^(1/n)-(x*y)^(1/n)
sage: assume(real(x) > 0, real(y) > 0)
sage: f.simplify()
x^(1/n)*y^(1/n) - (x*y)^(1/n)

[roed314]

Reviewer: David Roe

[orlitzky]

comment:4

Thanks, I'm building beta9 already and will refresh the patch ASAP.

[orlitzky]

Updated patch against 5.0.beta9

[orlitzky]

Attachment: sage-trac_12650.patch.gz

Changes from the previous patch

[orlitzky]

comment:5

Attachment: sage-trac_12650-review.patch.gz

This turned out to be an easy fix: simplify_log was setting the simplification domain to 'real' before doing its thing. Leaving it 'complex' doesn't hurt anything else, but fixes the Wester test.

It is likely that users will want to set the simplification domain to 'real' in some cases, but doing it sneakily like that is a bad way to go about it. Once I'm done with this and full_simplify, I'll probably go back and add a domain parameter to all of the simplification functions to allow the user a choice.

[kcrisman]

comment:6

I don't think this is the right fix.

• Breaks behavior for no reason
• Breaks with our recent trajectory of adding access to Maxima stuff and granularity
• Makes users have to do much more than needed for the super-basic simplifications that Maxima did
  Now, maybe we should do more in simplify. But it seems sort of redundant to remove radcan from simplify_full and then add everything else here anyway.

I think that it might make sense to look over the Maxima documentation and code very carefully and determine if something like the factorial simplification truly is the same in all cases, like it would be with expand. Otherwise it seems unwise to make this change.

I'm especially mystified as to what the difference between simplify and simplify_full should be under this regime.

[loefflerd]

comment:7

Apply sage-trac_12650.patch

(for the patchbot)

[orlitzky]

comment:8

Replying to @kcrisman:

I don't think this is the right fix.

• Breaks behavior for no reason
• Breaks with our recent trajectory of adding access to Maxima stuff and granularity
• Makes users have to do much more than needed for the super-basic simplifications that Maxima did
  Now, maybe we should do more in simplify. But it seems sort of redundant to remove radcan from simplify_full and then add everything else here anyway.

I think that it might make sense to look over the Maxima documentation and code very carefully and determine if something like the factorial simplification truly is the same in all cases, like it would be with expand. Otherwise it seems unwise to make this change.

I'm especially mystified as to what the difference between simplify and simplify_full should be under this regime.

I brought this up in my initial message to sage-devel, but didn't get any comments. I agree it's not perfect, but,

• Most of the simplifications we do are pretty basic anyway. I think it makes sense to have simplify() actually attempt something. If we ever discover that the simplifications in simplify() are misbehaving, they can just be removed.
• full_simplify() used to make two calls to simplify_rational() In Remove simplify_radical() from simplify_full() #12737 I've got it making two calls to simplify() so it isn't completely redundant even with unsafe=False.
• New simplifications can be added to full_simplify(), for example exp(I*pi*n).simplify_exp() doesn't simplify #11785. And maybe we could allow to take a complexity function as an argument ala Mathematica.

I am open to other ideas, though. I personally would be satisfied with full_simplify() if it were safe to call and I didn't have to define my own safe_simplify().

[kcrisman]

comment:9

Unfortunately I don't have time right now to discuss this more. I agree that it is frustrating that sage-devel folks haven't responded. I suspect that if behavior actually changes, there would be more complaints :)

Oh, simplify does simplify a few things. Not many, but you can grep through the doc to find them - notably, if you use an assumption about n being an integer, sin(n*pi) becomes 0.

I believe that the two simplify_rational guys were added to take care of a specific simplification which this did - sometimes Maxima is weird like that, though I suppose that is not unique. I would check with hg blame or something where this was added, just on the off chance someone didn't add a doctest for it. Maybe in the meantime Maxima got smarter on this one...

Deciding which simplifications were 'safe' is probably the first step. Under what circumstances all the other ones could go wrong, I mean, if any.

[orlitzky]

comment:10

No problem. I would suggest removing simplify_radical() from full_simplify() entirely if it didn't introduce a misnomer. Maybe radicals=True is the best way to do it, leaving simplify() as-is but making full_simplify() useful again.

[orlitzky]

comment:11

Replying to @orlitzky:

No problem. I would suggest removing simplify_radical() from full_simplify() entirely if it didn't introduce a misnomer. Maybe radicals=True is the best way to do it, leaving simplify() as-is but making full_simplify() useful again.

Now that I think about it, since I don't consider radcan a simplification, I'm OK with just removing it from full_simplify(). Would you be alright with that? I can ask on sage-devel again, and I don't think many doctests depend on it.

[kcrisman]

comment:12

I think this might be a way to deal with this, given that others also feel this way (e.g., Paul Zimmerman). But that should be at that other ticket, not here.

[orlitzky]

comment:13

Well, it turns out simplify_rational() is not so safe either #12794... ugh =)

[orlitzky]

comment:14

I light of #12794 and #12795, we should probably leave this alone.

[orlitzky]

comment:15

Hmm, we can close this. Can someone give it a positive review?

[kcrisman]

comment:16

I'm happy to do so, but not because of #12794 and #12795 (see my comments there, thanks for reminding me of them).

I'm going to have some time at Sage Days next week to think about this whole simplification business, so stay tuned with respect to #12737.

[rwst]

comment:17

Appears to have been forgotten.

[kcrisman]

Changed author from Michael Orlitzky to none

[kcrisman]

Changed reviewer from David Roe to MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan

[kcrisman]

Changed reviewer from MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan to Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan