invalid simplification of complex logarithm

[orlitzky]

As pointed out by J.H. Davenport in this sage-support thread,

http://groups.google.com/group/sage-support/browse_thread/thread/f9d2209041775c3e

the following (invalid) log simplification is made:

sage: t = var('t')                        
sage: assume(t, 'complex')               
sage: assumptions()                      
[t is complex]
sage: f = (1/2)*log(2*t) + (1/2)*log(1/t)
sage: f.full_simplify()                  
1/2*log(2)

When, for example, t=-1,

sage: f(t = -1)
I*pi + 1/2*log(2)

The assumption that t is complex is not necessary here, but simplify should definitely not ignore the imaginary part when dealing with complex functions.

Depends on #12737

Upstream: Reported upstream. No feedback yet.

Component: symbolics

Author: Michael Orlitzky

Branch/Commit: u/mjo/ticket/12322 @ 55eb0aa

Reviewer: Marc Mezzarobba

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

[kcrisman]

comment:2

Well, simplification is just that - simplification. You could investigate each of the various parts of full_simplify, but I suspect that log_simplify is the culprit, and Maxima's documentation on the function we use for that probably is clear that this kind of thing can happen. Of course, there could be a Maxima bug lurking as well...

[orlitzky]

comment:3

Replying to @kcrisman:

The simplification should always be equivalent to the original expression, though, or it isn't a simplification, it's a truncation (or something else). It doesn't make much sense to have a simplify() that doesn't return something equivalent to its argument: if I don't have to give you the right answer, I can simplify anything to zero!

I did check simplify_log() back when this was reported, and that wasn't the culprit. I dug a little deeper and found that it's simplify_exp() or simplify_radical() that does it. They both use the same maxima function. From the docs:

ALGORITHM:

       This uses the Maxima "radcan()" command. From the Maxima
       documentation: "All functionally equivalent forms are mapped into a
       unique form. For a somewhat larger class of expressions, produces a
       regular form. Two equivalent expressions in this class do not
       necessarily have the same appearance, but their difference can be
       simplified by radcan to zero. For some expressions radcan is quite
       time consuming. This is the cost of exploring certain relationships
       among the components of the expression for simplifications based on
       factoring and partial fraction expansions of exponents."

That, at least, doesn't make any mention of invalid simplifications.

[kcrisman]

comment:4

Ah, radcan. Our favorite problem in Sage simplification - see #8497 for the last time this became a brouhaha. See #11912 for the followup to that, and #11668 for why this probably is happening (temporarily going to reals for the domain), and #3520 for yet another example.

Basically, we have a difference of opinion between some users and Maxima as to what a simplification is. Is it a simplification of symbolic expressions which may be multivalued, or a simplification of functions?

Good luck finding a resolution; I have no good ideas on these issues right now, having wrestled with the other tickets in vain. I think it would be unfortunate to remove radcan from full simplification.

[orlitzky]

comment:5

I'm of the opinion that a simplify that sometimes makes invalid simplifications is like a car whose brakes work only 9/10 times. It's better than a car with no brakes, I guess, but unless they work essentially 100% of the time, I'm walking.

How do I as a user know whether or not I can trust the result? Can I trust the results for e.g. publication (or even homework)? Or should I treat them like random_element()? In some cases the results can be checked by hand, but in many, they can't -- that's why I'm using sage in the first place!

Anyway, thanks for finding those other tickets for me. I'm ranting in general and not at you =) I'll read through them all when I have some free time and coffee. Maybe there's a way to make everyone happy.

[kcrisman]

comment:6

Hee hee :)

Again, the real problem is that we are trying to treat "expressions" as "functions", I think. You should feel free to create a Maxima-only example and ask their list; I'd be intrigued to see what they say. Again, I have not looked into this closely; it could be an actual bug.

[orlitzky]

Upstream: Reported upstream. Little or no feedback.

[orlitzky]

comment:7

I reported this upstream at,

https://sourceforge.net/tracker/?func=detail&aid=3476031&group_id=4933&atid=104933

[orlitzky]

Doctest the fix from #12737.

[orlitzky]

comment:8

Attachment: sage-trac_12322.patch.gz

This is undoubtedly what the doctest will look like, but it probably doesn't make much sense to review this until #12737 is finished.

[orlitzky]

Author: Michael Orlitzky

[orlitzky]

Dependencies: #12737

[roed314]

Changed upstream from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.

[kcrisman]

comment:10

This probably needs an update about the word 'unsafe', that having left #12737. Trivial to change. I've also inquired again about the upstream, in case this ends up being orthogonal.

[orlitzky]

comment:13

I must have missed this in the great trac email blackout. I'll try to figure out the git workflow and update the patch.

[orlitzky]

Branch: u/mjo/ticket/12322

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

[36eff6f](https://github.com/sagemath/sagetrac-mirror/commit/36eff6f)

Trac #12322: Add a doctest for the correct behavior introduced in trac #12737.

[sagetrac-git]

Commit: 36eff6f

[jdemeyer]

comment:20

Needs to be rebased.

[sagetrac-git]

Changed commit from 36eff6f to 55eb0aa

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. Recent commits:

55eb0aa	Trac #12322: Add a doctest for the correct behavior introduced in trac #12737.

[orlitzky]

comment:22

Replying to @sagetrac-git:

55eb0aa	Trac #12322: Add a doctest for the correct behavior introduced in trac #12737.

Nice, git will let me be bad. Since this is a trivial patch, I rebased (that's git rebase) on top of master.

[mezzarobba]

comment:23

I'd remove the call to assume() from the test, since I think there is a consensus that symbolic variables are complex by default. What do you think?

[orlitzky]

comment:24

The symbolic domain situation is pretty muddy. We want variables to be complex by default, and we set the Maxima "domain" to complex for each new symbolic variable. But, that isn't always enough to affect simplifications. Some parts of Maxima use the "domain", others use assumptions, some use neither, and I'll bet one or two use both.

The complex assumption in the test case wasn't strictly necessary to make it fail, but it highlights the fact that this is a bug only when t is complex, and makes it clear that we have done all we can to indicate to sage that t is complex.

[mezzarobba]

Reviewer: Marc Mezzarobba

[mezzarobba]

comment:25

Replying to @orlitzky:

The complex assumption in the test case wasn't strictly necessary to make it fail, but it highlights the fact that this is a bug only when t is complex, and makes it clear that we have done all we can to indicate to sage that t is complex.

Sure; I am only a bit concerned that having the assumption in the test gives the impression that the variable is real by default. But it's no big deal.