Remove simplify_radical() from simplify_full()

[orlitzky]

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
• Symbolic simplification error #11934 - Symbolic simplification error
• invalid simplification of complex logarithm #12322 - invalid simplification of complex logarithm
• Ask Sage 1983
  And maybe more. Here are some related tickets.
• Clarify simplify_radical and Maxima's radcan #11912 is related, though about simplify_radical specifically.
• abs(a+b)^2 == (a+b)^2 #11668 - abs(a+b)^2 == (a+b)^2 is now fixed by Be more careful about setting the Maxima 'domain' #12780
• Doctest: Immediate simplifications of symbolic powers #14305 and Add simplify_real method to symbolic expressions #14630 are tangentially related

Apply attachment: sage-trac_12737-rebased.patch and attachment: trac_12737-de.patch

CC: @jvkersch @sagetrac-navigium

Component: symbolics

Author: Michael Orlitzky

Reviewer: Karl-Dieter Crisman, Beni Keller

Merged: sage-5.12.beta0

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

[orlitzky]

Add the flag and update doctests.

[orlitzky]

comment:1

Attachment: sage-trac_12737.patch.gz

[nbruin]

comment:2

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?).

[orlitzky]

comment:3

Replying to @nbruin:

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?).

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.

[kcrisman]

comment:4

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?).

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 consider x^2/x equivalent to x.

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

[orlitzky]

comment:5

Replying to @kcrisman:

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?).

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 consider x^2/x equivalent to x.

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

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.

[orlitzky]

comment:6

There is not consensus that this is even a good idea, so I'll leave it alone until we can discuss it further.

[orlitzky]

Changed dependencies from #12650 to none

[orlitzky]

comment:7

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.

[orlitzky]

Author: Michael Orlitzky

[orlitzky]

Same patch with the functional.py doctest factored out

[orlitzky]

comment:8

Attachment: sage-trac_12737-remove_radcan.patch.gz

I moved the common doctest to #12845 and removed it from this patch.

[orlitzky]

Dependencies: #12845

[kcrisman]

comment:9

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!

[kcrisman]

Changed dependencies from #12845 to none

[kcrisman]

Reviewer: Karl-Dieter Crisman

[sagetrac-navigium]

comment:22

I'm at the moment unsure how to fix this. I guess the first example that fails could just be removed since it can be numerically evaluated. But I think the second example should remain in the tutorial and should be fixed. At the moment I don't have the time to look into it. I won't be around till mid August.

@kcrisman Ich habe es vor allem als Vereinfachung bezeichnet, da es die notwendige Umformung ist, welche man für das Lösen von Exponentialgleichungen benötigt. Aus diesem Grund halte ich es auch für wichtig, dass man erwähnt, wie man eine solche Umformung machen kann.

Man könnte es aber natürlich umbennen. Ich würde es als "Zerlegung durch Benutzen der Logarithmengesetze" oder ähnlich nennen.

Natürlich wäre es schon, wenn man simplify_radical nicht separat aufführen muss, um die Sache einfach zu halten. Wenn es aber aus simplify_full fliegt, müsste man es wohl erwähnen.

[kcrisman]

comment:23

@kcrisman Ich habe es vor allem als Vereinfachung bezeichnet, da es die notwendige Umformung ist, welche man für das Lösen von Exponentialgleichungen benötigt. Aus diesem Grund halte ich es auch für wichtig, dass man erwähnt, wie man eine solche Umformung machen kann.

Man könnte es aber natürlich umbennen. Ich würde es als "Zerlegung durch Benutzen der Logarithmengesetze" oder ähnlich nennen.

Eigentlich genügt das!

Natürlich wäre es schon, wenn man simplify_radical nicht separat aufführen muss, um die Sache einfach zu halten. Wenn es aber aus simplify_full fliegt, müsste man es wohl erwähnen.

Tja...

Michael, I'll try to keep this on the front burner for finding a good fix - navigium has a good phrase we can use, and I don't really see any other way to fix this, since it does happen to be very convenient that we can use radcan in some cases to do log expansion.

[orlitzky]

comment:24

It is regrettable that there's no other way to get log simplification of things like log(8)/log(2), but simplify_radical() will do the same thing even when the argument to the log function is a complex function of, say, z.

It'd be nice to have something that worked for constants at least, but avoided e.g. #12322.

[kcrisman]

Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Beni Keller

[kcrisman]

comment:25

I'd appreciate a quick review from navigium as to whether my German syntax is 100% correct, but this should take care of it, and conforms to what he suggested as a solution.

Patchbot, apply sage-trac_12737-rebased.patch and trac_12737-de.patch

[kcrisman]

Attachment: trac_12737-de.patch.gz

[kcrisman]

comment:26

Patchbot, apply sage-trac_12737-rebased.patch and trac_12737-de.patch

[sagetrac-navigium]

comment:27

I think that's a great way to solve my document. Now the readers even get something extra because they are taught how to apply log rules both ways. Thanks @kcrisman for solving it.

[kcrisman]

comment:28

Great. Now someone (other than me) just needs to check that it passes tests and they can set it to positive review.

[jvkersch]

comment:29

make ptestlong does not report any errors so I'm setting this to positive review.

[jdemeyer]

Merged: sage-5.12.beta0

[kcrisman]

comment:32

Updates:

Removed

• inconsistency in simplify_radical #3520 - inconsistency in simplify_radical
  from description, as that was not about simplify_full to start with.

I posted updates on the ask.sagemath questions just now.

#11934 needs review.

#12322 needs to be reworded since we didn't use the "unsafe" parameter in simplify_full.