Opened 10 years ago

Closed 8 years ago

# Perform safe simplifications in Expression.simplify()

Reported by: Owned by: mjo mjo major sage-duplicate/invalid/wontfix symbolics Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan N/A

### Description

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. #3520 - inconsistency in simplify_radical
2. #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.

### comment:1 Changed 10 years ago by mjo

• Authors set to Michael Orlitzky
• Status changed from new to needs_review

### comment:2 Changed 10 years ago by roed

• Status changed from needs_review to needs_work

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

### comment:3 Changed 10 years ago by roed

• Reviewers set to David Roe

### comment:4 Changed 10 years ago by mjo

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

### Changed 10 years ago by mjo

Updated patch against 5.0.beta9

### Changed 10 years ago by mjo

Changes from the previous patch

### comment:5 Changed 10 years ago by mjo

• Status changed from needs_work to needs_review

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.

### comment:6 follow-up: ↓ 8 Changed 10 years ago by kcrisman

I don't think this is the right fix.

• Breaks behavior for no reason
• 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.

### comment:7 Changed 10 years ago by davidloeffler

Apply sage-trac_12650.patch

(for the patchbot)

### comment:8 in reply to: ↑ 6 Changed 10 years ago by mjo

I don't think this is the right fix.

• Breaks behavior for no reason
• 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 #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 #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()`.

### comment:9 Changed 10 years ago by kcrisman

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.

### comment:10 follow-up: ↓ 11 Changed 10 years ago by mjo

• Status changed from needs_review to needs_info

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.

### comment:11 in reply to: ↑ 10 Changed 10 years ago by mjo

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.

### comment:12 Changed 10 years ago by kcrisman

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.

### comment:13 Changed 10 years ago by mjo

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

### comment:14 Changed 10 years ago by mjo

• Milestone changed from sage-5.0 to sage-duplicate/invalid/wontfix

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

### comment:15 Changed 9 years ago by mjo

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

### comment:16 Changed 9 years ago by kcrisman

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.

### comment:17 Changed 8 years ago by rws

• Status changed from needs_info to positive_review

Appears to have been forgotten.

### comment:18 Changed 8 years ago by vbraun

• Resolution set to invalid
• Status changed from positive_review to closed

### comment:19 Changed 8 years ago by kcrisman

• Authors Michael Orlitzky deleted
• Reviewers changed from David Roe to MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan

### comment:20 Changed 8 years ago by kcrisman

• Reviewers changed from MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan to Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan
Note: See TracTickets for help on using tickets.