#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.
- #3520 - inconsistency in simplify_radical
- #12322 - invalid simplification of complex logarithm
- 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)
Change History (22)
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 Changed 7 years ago by
- Status changed from needs_review to needs_work
comment:3 Changed 7 years ago by
- Reviewers set to David Roe
comment:4 Changed 7 years ago by
Thanks, I'm building beta9 already and will refresh the patch ASAP.
comment:5 Changed 7 years ago by
- 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 7 years ago by
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 7 years ago by
Apply sage-trac_12650.patch
(for the patchbot)
comment:8 in reply to: ↑ 6 Changed 7 years ago by
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 removeradcan
fromsimplify_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
andsimplify_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 insimplify()
are misbehaving, they can just be removed. full_simplify()
used to make two calls tosimplify_rational()
In #12737 I've got it making two calls tosimplify()
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 7 years ago by
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 7 years ago by
- 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 7 years ago by
Replying to mjo:
No problem. I would suggest removing
simplify_radical()
fromfull_simplify()
entirely if it didn't introduce a misnomer. Mayberadicals=True
is the best way to do it, leavingsimplify()
as-is but makingfull_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 7 years ago by
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 7 years ago by
Well, it turns out simplify_rational()
is not so safe either #12794... ugh =)
comment:14 Changed 7 years ago by
- Milestone changed from sage-5.0 to sage-duplicate/invalid/wontfix
comment:15 Changed 6 years ago by
Hmm, we can close this. Can someone give it a positive review?
comment:16 Changed 6 years ago by
comment:17 Changed 5 years ago by
- Status changed from needs_info to positive_review
Appears to have been forgotten.
comment:18 Changed 5 years ago by
- Resolution set to invalid
- Status changed from positive_review to closed
comment:19 Changed 5 years ago by
- Reviewers changed from David Roe to MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan
comment:20 Changed 5 years ago by
- Reviewers changed from MIchael Orlitzky, Karl-Dieter Crisman, Ralf Stephan to Michael Orlitzky, Karl-Dieter Crisman, Ralf Stephan
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.
Before your patch, the first f.simplify() didn't do much: