Opened 11 years ago

Closed 9 years ago

# invalid simplification of complex logarithm

Reported by: Owned by: mjo burcin major sage-6.1 symbolics Michael Orlitzky Marc Mezzarobba Reported upstream. No feedback yet. u/mjo/ticket/12322 55eb0aa40402e5fc61a4f7294420c4df3d5e303f #12737

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

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.

### comment:1 Changed 11 years ago by mjo

• Description modified (diff)

### comment:2 follow-up: ↓ 3 Changed 11 years ago by kcrisman

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

### comment:3 in reply to: ↑ 2 Changed 11 years ago by mjo

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

### comment:4 Changed 11 years ago by kcrisman

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.

### comment:5 Changed 11 years ago by mjo

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.

### comment:6 Changed 11 years ago by kcrisman

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.

### comment:7 Changed 11 years ago by mjo

• Report Upstream changed from N/A to Reported upstream. Little or no feedback.

I reported this upstream at,

### Changed 10 years ago by mjo

Doctest the fix from #12737.

### comment:8 Changed 10 years ago by mjo

• Authors set to Michael Orlitzky
• Dependencies set to #12737
• Status changed from new to needs_review

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

### comment:9 Changed 10 years ago by roed

• Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.

### comment:10 Changed 9 years ago by kcrisman

• Status changed from needs_review to needs_work

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.

### comment:11 Changed 9 years ago by jdemeyer

• Milestone changed from sage-5.11 to sage-5.12

### comment:12 Changed 9 years ago by jdemeyer

• Description modified (diff)

### comment:13 Changed 9 years ago by mjo

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

### comment:14 Changed 9 years ago by mjo

• Branch set to u/mjo/ticket/12322

### comment:15 Changed 9 years ago by git

• Commit set to 36eff6f7a9a1f2da5dfafaa4a41ec4cc4ba068bf

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

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

### comment:16 Changed 9 years ago by mjo

• Status changed from needs_work to needs_review

### comment:17 Changed 9 years ago by jdemeyer

• Milestone changed from sage-5.13 to sage-6.0

### comment:18 Changed 9 years ago by jdemeyer

• Description modified (diff)

### comment:19 Changed 9 years ago by vbraun_spam

• Milestone changed from sage-6.0 to sage-6.1

### comment:20 Changed 9 years ago by jdemeyer

• Status changed from needs_review to needs_work

Needs to be rebased.

### comment:21 follow-up: ↓ 22 Changed 9 years ago by git

• Commit changed from 36eff6f7a9a1f2da5dfafaa4a41ec4cc4ba068bf to 55eb0aa40402e5fc61a4f7294420c4df3d5e303f

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.`

### comment:22 in reply to: ↑ 21 Changed 9 years ago by mjo

• Status changed from needs_work to needs_review

 ​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.

### comment:23 Changed 9 years ago by mmezzarobba

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?

### comment:24 follow-up: ↓ 25 Changed 9 years ago by mjo

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.

### comment:25 in reply to: ↑ 24 Changed 9 years ago by mmezzarobba

• Reviewers set to Marc Mezzarobba
• Status changed from needs_review to positive_review

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.