Opened 8 years ago
Closed 6 years ago
#12322 closed defect (fixed)
invalid simplification of complex logarithm
Reported by:  mjo  Owned by:  burcin 

Priority:  major  Milestone:  sage6.1 
Component:  symbolics  Keywords:  
Cc:  Merged in:  
Authors:  Michael Orlitzky  Reviewers:  Marc Mezzarobba 
Report Upstream:  Reported upstream. No feedback yet.  Work issues:  
Branch:  u/mjo/ticket/12322  Commit:  55eb0aa40402e5fc61a4f7294420c4df3d5e303f 
Dependencies:  #12737  Stopgaps: 
Description (last modified by )
As pointed out by J.H. Davenport in this sagesupport thread,
http://groups.google.com/group/sagesupport/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.
Attachments (1)
Change History (27)
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 followup: ↓ 3 Changed 8 years ago by
comment:3 in reply to: ↑ 2 Changed 8 years ago by
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.
comment:4 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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 Maximaonly 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 8 years ago by
 Report Upstream changed from N/A to Reported upstream. Little or no feedback.
I reported this upstream at,
https://sourceforge.net/tracker/?func=detail&aid=3476031&group_id=4933&atid=104933
comment:8 Changed 8 years ago by
 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 8 years ago by
 Report Upstream changed from Reported upstream. Little or no feedback. to Reported upstream. No feedback yet.
comment:10 Changed 6 years ago by
 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 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:12 Changed 6 years ago by
 Description modified (diff)
comment:13 Changed 6 years ago by
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 6 years ago by
 Branch set to u/mjo/ticket/12322
comment:15 Changed 6 years ago by
 Commit set to 36eff6f7a9a1f2da5dfafaa4a41ec4cc4ba068bf
comment:16 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:17 Changed 6 years ago by
 Milestone changed from sage5.13 to sage6.0
comment:18 Changed 6 years ago by
 Description modified (diff)
comment:19 Changed 6 years ago by
 Milestone changed from sage6.0 to sage6.1
comment:20 Changed 6 years ago by
 Status changed from needs_review to needs_work
Needs to be rebased.
comment:21 followup: ↓ 22 Changed 6 years ago by
 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 6 years ago by
 Status changed from needs_work to needs_review
comment:23 Changed 6 years ago by
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 followup: ↓ 25 Changed 6 years ago by
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 6 years ago by
 Reviewers set to Marc Mezzarobba
 Status changed from needs_review to positive_review
Replying to mjo:
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 thatt
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.
comment:26 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Well, simplification is just that  simplification. You could investigate each of the various parts of
full_simplify
, but I suspect thatlog_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...