Opened 3 years ago

Closed 7 months ago

#12322 closed defect (fixed)

invalid simplification of complex logarithm

Reported by: mjo Owned by: burcin
Priority: major Milestone: sage-6.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 (Commits) Commit: 55eb0aa40402e5fc61a4f7294420c4df3d5e303f
Dependencies: #12737 Stopgaps:

Description (last modified by jdemeyer)

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.

Attachments (1)

sage-trac_12322.patch (961 bytes) - added by mjo 2 years ago.
Doctest the fix from #12737.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by mjo

  • Description modified (diff)

comment:2 follow-up: Changed 3 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 3 years ago by mjo

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 3 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 3 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 3 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 3 years ago by mjo

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

Changed 2 years ago by mjo

Doctest the fix from #12737.

comment:8 Changed 2 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 2 years ago by roed

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

comment:10 Changed 14 months 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 12 months ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:12 Changed 8 months ago by jdemeyer

  • Description modified (diff)

comment:13 Changed 8 months 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 8 months ago by mjo

  • Branch set to u/mjo/ticket/12322

comment:15 Changed 8 months ago by git

  • Commit set to 36eff6f7a9a1f2da5dfafaa4a41ec4cc4ba068bf

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

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

comment:16 Changed 8 months ago by mjo

  • Status changed from needs_work to needs_review

comment:17 Changed 8 months ago by jdemeyer

  • Milestone changed from sage-5.13 to sage-6.0

comment:18 Changed 8 months ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 8 months ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:20 Changed 7 months ago by jdemeyer

  • Status changed from needs_review to needs_work

Needs to be rebased.

comment:21 follow-up: Changed 7 months 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:

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

comment:22 in reply to: ↑ 21 Changed 7 months ago by mjo

  • Status changed from needs_work to needs_review

Replying to git:

55eb0aaTrac #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 7 months 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: Changed 7 months 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 7 months ago by mmezzarobba

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

comment:26 Changed 7 months ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.