Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#12650 closed enhancement (invalid)

Perform safe simplifications in Expression.simplify()

Reported by: mjo Owned by: mjo
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: symbolics Keywords:
Cc: Merged in:
Authors: Reviewers: Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

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.

Attachments (2)

sage-trac_12650.patch (4.1 KB) - added by mjo 8 years ago.
Updated patch against 5.0.beta9
sage-trac_12650-review.patch (939 bytes) - added by mjo 8 years ago.
Changes from the previous patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by mjo

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

comment:2 Changed 8 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 8 years ago by roed

  • Reviewers set to David Roe

comment:4 Changed 8 years ago by mjo

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

Changed 8 years ago by mjo

Updated patch against 5.0.beta9

Changed 8 years ago by mjo

Changes from the previous patch

comment:5 Changed 8 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: Changed 8 years ago by 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.

comment:7 Changed 8 years ago by davidloeffler

Apply sage-trac_12650.patch

(for the patchbot)

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

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 #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 8 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: Changed 8 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 8 years ago by mjo

Replying to 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 8 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 8 years ago by mjo

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

comment:14 Changed 8 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 6 years ago by mjo

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

comment:16 Changed 6 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 5 years ago by rws

  • Status changed from needs_info to positive_review

Appears to have been forgotten.

comment:18 Changed 5 years ago by vbraun

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

comment:19 Changed 5 years ago by kcrisman

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

comment:20 Changed 5 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.