Opened 7 years ago
Last modified 6 years ago
#12737 closed enhancement
Remove simplify_radical() from simplify_full() — at Version 10
Reported by: | mjo | Owned by: | mjo |
---|---|---|---|
Priority: | major | Milestone: | sage-5.12 |
Component: | symbolics | Keywords: | |
Cc: | jvkersch, navigium | Merged in: | |
Authors: | Michael Orlitzky | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12845 | Stopgaps: |
Description (last modified by )
There are a number of tickets open due to the use of simplify_radical()
in simplify_full()
.
Removing it would fix at least,
- Ask Sage 767
- #3520 - inconsistency in simplify_radical
- #11934 - Symbolic simplification error
- #12322 - invalid simplification of complex logarithm
- Ask Sage 1983
And maybe more.
Change History (12)
Changed 7 years ago by
comment:1 Changed 7 years ago by
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 7 years ago by
It's not entirely clear to me which simplifications are "safe" and which are not. Simplifying x^2/x
to x
is also not "safe" in the sense that equality does not hold for x=0
. In many computer algebra systems, acceptable simplifications are not "safe" in this respect. By including a default unsafe=False
I'm afraid you'll be raising expectations to a level that Sage does not attain (yet?).
comment:3 in reply to: ↑ 2 ; follow-up: ↓ 4 Changed 7 years ago by
Replying to nbruin:
It's not entirely clear to me which simplifications are "safe" and which are not. Simplifying
x^2/x
tox
is also not "safe" in the sense that equality does not hold forx=0
. In many computer algebra systems, acceptable simplifications are not "safe" in this respect. By including a defaultunsafe=False
I'm afraid you'll be raising expectations to a level that Sage does not attain (yet?).
It's completely heuristic: the four I chose nobody seems to have a problem with. simplify_radical
, on the other hand, wreaks havoc on trivial functions and has been the cause of numerous bug reports and mailing list discussions.
In simplify
, after #12650, I say that a safe simplification is one "for which we are reasonably sure that the input will be considered equivalent to the output." That's probably the best we can do for now, and I'm reasonably sure that most people would consider x^2/x
equivalent to x
.
As it stands, I think full_simplify
sounds safe so people will assume that anyway. This fix, while not perfect, at least improves things.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 7 years ago by
It's not entirely clear to me which simplifications are "safe" and which are not. Simplifying
x^2/x
tox
is also not "safe" in the sense that equality does not hold forx=0
. In many computer algebra systems, acceptable simplifications are not "safe" in this respect. By including a defaultunsafe=False
I'm afraid you'll be raising expectations to a level that Sage does not attain (yet?).It's completely heuristic: the four I chose nobody seems to have a problem with.
Only in that we couldn't find Trac tickets about them.
simplify_radical
, on the other hand, wreaks havoc on trivial functions and has been the cause of numerous bug reports and mailing list discussions.In
simplify
, after #12650, I say that a safe simplification is one "for which we are reasonably sure that the input will be considered equivalent to the output." That's probably the best we can do for now, and I'm reasonably sure that most people would considerx^2/x
equivalent tox
.
I'm reasonably sure that no mathematician would consider them equivalent unless you add "almost everywhere". Simplification simplifies, hence makes it not actually the same (at least potentially). This shouldn't be controversial.
As it stands, I think
full_simplify
sounds safe so people will assume that anyway. This fix, while not perfect, at least improves things.
Well, everything sounds safe unless you put in the word "unsafe".
I'm pretty confused as to the relation of this to #12650. See my comments there.
I'm sympathetic to doing something about radcan
, but simplification is simplification, not identity.
Doesn't it make more sense to either document fully what can go wrong with simplify_full
, or to add a flag simplify_radicals
or something? I think that changing behavior in this case is probably the best compromise, but probably unsafe
is sending the wrong message. There isn't anything unsafe about radcan
, it's just a different point of view than ours - symbolic expressions versus functions. Spend some time on the Maxima list :)
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to kcrisman:
It's not entirely clear to me which simplifications are "safe" and which are not. Simplifying
x^2/x
tox
is also not "safe" in the sense that equality does not hold forx=0
. In many computer algebra systems, acceptable simplifications are not "safe" in this respect. By including a defaultunsafe=False
I'm afraid you'll be raising expectations to a level that Sage does not attain (yet?).It's completely heuristic: the four I chose nobody seems to have a problem with.
Only in that we couldn't find Trac tickets about them.
simplify_radical
, on the other hand, wreaks havoc on trivial functions and has been the cause of numerous bug reports and mailing list discussions.In
simplify
, after #12650, I say that a safe simplification is one "for which we are reasonably sure that the input will be considered equivalent to the output." That's probably the best we can do for now, and I'm reasonably sure that most people would considerx^2/x
equivalent tox
.I'm reasonably sure that no mathematician would consider them equivalent unless you add "almost everywhere". Simplification simplifies, hence makes it not actually the same (at least potentially). This shouldn't be controversial.
Before our discussion on another one of these tickets, I had assumed it was not controversial that simplification had to return an equivalent expression. So it is at least a little controversial.
As it stands, I think
full_simplify
sounds safe so people will assume that anyway. This fix, while not perfect, at least improves things.Well, everything sounds safe unless you put in the word "unsafe".
I'm pretty confused as to the relation of this to #12650. See my comments there.
I'm sympathetic to doing something about
radcan
, but simplification is simplification, not identity.Doesn't it make more sense to either document fully what can go wrong with
simplify_full
, or to add a flagsimplify_radicals
or something? I think that changing behavior in this case is probably the best compromise, but probablyunsafe
is sending the wrong message. There isn't anything unsafe aboutradcan
, it's just a different point of view than ours - symbolic expressions versus functions. Spend some time on the Maxima list :)
All expressions in sage are callable like functions, so if you have both radicals and a variable in an expression, simplify_radical()
is unsafe. I.e. you're probably gonna get the wrong answer. I think marking it as unsafe is more likely to catch someone's attention than asking him to read the documentation. I almost certainly used full_simplify()
for years without doing so, since I thought I knew what it was doing and had used similar functions in e.g. Mathematica. I think the problem is that it's called "simplification," not that it exists.
Either way, f.full_simplify(simplify_radicals=True)
is not much better than f.full_simplify().simplify_radical()
... the keyword argument is the easy way out, but I feel like it should control more than one method call if we do it. And if we remove the unsafe simplifications from full_simplify()
, that seems to make simplify()
redundant, too. That's the dual situation to what we'd have after #12650, but at least we could add more stuff to full_simplify()
in the future to change that.
comment:6 Changed 7 years ago by
- Status changed from needs_review to needs_info
There is not consensus that this is even a good idea, so I'll leave it alone until we can discuss it further.
comment:7 Changed 7 years ago by
- Dependencies #12650 deleted
- Description modified (diff)
- Status changed from needs_info to needs_review
- Summary changed from Add an `unsafe` argument to Expression.simplify_full() to Remove simplify_radical() from simplify_full()
This should be a more straight-forward proposal. I just removed simplify_radical()
from simplify_full()
and fixed a few doctests (details in the commit message).
One of the doctest fixes is duplicated in #12780; the doctest is wrong (missing assumptions) but we're masking that fact at the moment.
comment:8 Changed 7 years ago by
- Dependencies set to #12845
I moved the common doctest to #12845 and removed it from this patch.
comment:9 Changed 7 years ago by
- Description modified (diff)
Lest it seem weird that I'm adding to the list in the description, let it be known that I haven't necessarily changed my mind here, but fairness dictates that I point this additional example out!
comment:10 Changed 6 years ago by
- Description modified (diff)
Add the flag and update doctests.