Sage: Ticket #7334: Sage cannot simplify sums of logarithms
https://trac.sagemath.org/ticket/7334
<p>
Currently there is no direct way in Sage to apply the transformation:
</p>
<pre class="wiki">log(x) + log(y) -> log(x*y)
</pre><p>
The attached patch fixes this by inserting a call to logcontract()
in the definition of simplify_radical.
</p>
<p>
Now the following works:
</p>
<pre class="wiki">sage: f = log(sqrt(2) - 1) + log(sqrt(2) + 1)
sage: f.simplify_full()
0
</pre><p>
But I'm not sure if this is the right place for this.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/7334
Trac 1.1.6whussWed, 28 Oct 2009 17:33:27 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7334-logcontract.patch</em>
</li>
</ul>
TicketwhussWed, 28 Oct 2009 17:33:44 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:1
https://trac.sagemath.org/ticket/7334#comment:1
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketkcrismanThu, 29 Oct 2009 18:06:13 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:2
https://trac.sagemath.org/ticket/7334#comment:2
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Hey, I love this idea! And the code seems fine on the face of it.
</p>
<p>
But probably it makes the most sense to make it separately available as a simplification, i.e. make a new method .simplify_log() or something. Exposing as many of these as possible is good for the (many) users who keep one wanting some, but not all simplifications, which I think is something people really like about Maxima (from reading their lists).
</p>
<p>
Anyway, then you could just call this wherever you think is best in the definition of .simplify_full(), which certainly should have this included. And one would want a (perhaps slightly complicated) example added to simplify_full() which shows that it's used correctly there as well.
</p>
Ticketrobert.marikSat, 07 Nov 2009 21:14:57 GMT
https://trac.sagemath.org/ticket/7334#comment:3
https://trac.sagemath.org/ticket/7334#comment:3
<p>
I also think that it is better to keep the function logcontract separately from simplify_full() since sometimes the contraction of logarithms is not what user wants. Consider please also option which allows to set logconfcoef as described in <a class="ext-link" href="http://maxima.sourceforge.net/docs/manual/en/maxima_14.html#Item_003a-logconcoeffp"><span class="icon"></span>Maxima</a>. This allows to contract expressions like log(x)+1/2*log(8) into log(x*sqrt(8)).
</p>
Ticketrobert.marikSat, 07 Nov 2009 21:23:38 GMT
https://trac.sagemath.org/ticket/7334#comment:4
https://trac.sagemath.org/ticket/7334#comment:4
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7334#comment:2" title="Comment 2">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Anyway, then you could just call this wherever you think is best in the definition of .simplify_full(), which certainly should have this included.
</p>
</blockquote>
<p>
Do not do it please. The user knows if he/she wants to contract logarithms or not and then he/she can run the coresponding method. If you include this as an automatical simplification in simplify_full, consider the following
</p>
<ul><li>domain of log(1-x)+log(1+x) is different from domain of log(1-x<sup>2)
</sup></li></ul><ul><li>you should add a function which expands logarithms
</li></ul><p>
Thanks
Robert
</p>
Ticketrobert.marikSat, 07 Nov 2009 23:19:21 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:5
https://trac.sagemath.org/ticket/7334#comment:5
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Attached new patch, apply only this patch.
</p>
<p>
Options which controls if simplify expressions like 1/2*log(x) or not has been introduced.
</p>
<p>
simplify_log now does not use radcan, it calls only logcontract in Maxima session. However, there are many aliases for radcan:
radical_simplify, simplify_radical, exp_simplify, simplify_exp
</p>
Ticketrobert.marikSat, 07 Nov 2009 23:19:42 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7344-logcontract-2.patch</em>
</li>
</ul>
TicketkcrismanSun, 08 Nov 2009 01:58:15 GMT
https://trac.sagemath.org/ticket/7334#comment:6
https://trac.sagemath.org/ticket/7334#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7334#comment:4" title="Comment 4">robert.marik</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7334#comment:2" title="Comment 2">kcrisman</a>:
</p>
<blockquote class="citation">
<p>
Anyway, then you could just call this wherever you think is best in the definition of .simplify_full(), which certainly should have this included.
</p>
</blockquote>
<p>
Do not do it please. The user knows if he/she wants to contract logarithms or not and then he/she can run the coresponding method. If you include this as an automatical simplification in simplify_full, consider the following
</p>
</blockquote>
<p>
I disagree. simplify_full is the sort of thing used by people who do NOT know if they want to contract - they want the simplest-looking form possible. In fact, these people usually use just simplify() first and then email sage-support complaining it doesn't do things like this :)
</p>
<p>
Anyone who is looking for something specific can use the specific wrappers for the Maxima simplifiers; the general user who is not actually interested in symbolics or niceties like domains (which presumably the other simplifiers also disrespect, e.g. x<strong>2/x is not x but presumably one of them does this and is part of simplify_full) needs a function which applies as much machinery as possible, and simplify_full is it.
</strong></p>
<p>
That said, wrapping more of the expanding functions is a very good idea! One could even have an "expand_full" function to complement the "simplify_full".
</p>
<p>
(Unfortunately, many users (including me) get tripped on on simplify versus expand linguistically, because in colloquial high school English they are often used interchangeably... sigh, but I'm just as guilty as anyone.)
</p>
Ticketrobert.marikSun, 08 Nov 2009 14:13:33 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7344-logcontract-3.patch</em>
</li>
</ul>
<p>
apply only this patch
</p>
Ticketrobert.marikSun, 08 Nov 2009 14:14:43 GMT
https://trac.sagemath.org/ticket/7334#comment:7
https://trac.sagemath.org/ticket/7334#comment:7
<p>
Added contraction of logarithms to simplify_full() and some more options.
</p>
Ticketrobert.marikSun, 08 Nov 2009 14:29:56 GMT
https://trac.sagemath.org/ticket/7334#comment:8
https://trac.sagemath.org/ticket/7334#comment:8
<p>
note that something does not work as expected due to recently fixed Maxima <a class="ext-link" href="http://sourceforge.net/tracker/index.php?func=detail&aid=2835634&group_id=4933&atid=104933"><span class="icon"></span>bug</a>.
</p>
<p>
As a particular example (log(5)-log(2)).logcontract() does not work now, (log(x)-log(y)).logcontract() works as expected.
</p>
TicketkcrismanTue, 10 Nov 2009 14:13:12 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:9
https://trac.sagemath.org/ticket/7334#comment:9
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Here are a few comments, which you can incorporate if you agree with them. Overall, though, a VERY well documented patch, which tells the user exactly what they can and cannot do with it, options, etc. Good work!
</p>
<ol><li>typo of "gowerns" instead of "governs" in line 5268
</li></ol><ol start="2"><li>Maybe the if coeff is not None: should be set off in a block? For better readability and logical flow.
</li></ol><ol start="3"><li>Perhaps
<pre class="wiki">sage: (log(5)-log(2)).simplify_log()
-log(2) + log(5)
</pre></li></ol><p>
should be included as a doctest, with a comment that this is not simplified (though it's not mathematically wrong, so I am okay with this being as is). This will also encourage us to upgrade Maxima :)
</p>
<ol start="4"><li>There are some duplicated doctests. Is this intentional (i.e., to show it won't simplify any more)? Since
<pre class="wiki">sage: x,y,t=var('x y t')
sage: f = log(x)+2*log(y)+1/2*log(t)
sage: f.simplify_log()
log(x*y^2) + 1/2*log(t)
sage: f
1/2*log(t) + log(x) + 2*log(y)
</pre></li></ol><p>
it must have some other rationale. Anyway, that should be clarified, or the duplicates should be removed, as this is confusing.
</p>
Ticketrobert.marikThu, 12 Nov 2009 10:50:19 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7334-logcontract-4.patch</em>
</li>
</ul>
<p>
Apply only this patch.
</p>
Ticketrobert.marikThu, 12 Nov 2009 10:52:34 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:10
https://trac.sagemath.org/ticket/7334#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<p>
Many thanks for reviewing and comments. New patch is there. It accepts your suggestions and adds more:
</p>
<p>
If we contract logarithms, we have to build the way back - I added expansion of logarithms.
</p>
<p>
I also updated simplification of rational functions and added one option to simplify_trig.
</p>
Ticketrobert.marikThu, 12 Nov 2009 10:55:52 GMT
https://trac.sagemath.org/ticket/7334#comment:11
https://trac.sagemath.org/ticket/7334#comment:11
<p>
And there was also no way to go back after expanding trigonometric function. I added interface to Maxima's trigreduce to this patch.
</p>
TicketburcinThu, 12 Nov 2009 11:08:37 GMTcc set
https://trac.sagemath.org/ticket/7334#comment:12
https://trac.sagemath.org/ticket/7334#comment:12
<ul>
<li><strong>cc</strong>
<em>fmaltey@…</em> added
</li>
</ul>
<p>
I really dislike the idea of adding a new function for each functionality of this kind. This makes it very hard for users to figure out the function name they need for a specific task.
</p>
<p>
We should be able to provide an interface to all these "rewriting" tasks through a few conceptually defined methods like rewrite(), expand() and combine()( or contract()?).
</p>
<p>
Francois Maltey had a proposal for a possible interface to all this. Maybe he can comment here, or we can discuss his proposal on sage-devel.
</p>
Ticketrobert.marikThu, 12 Nov 2009 13:06:30 GMT
https://trac.sagemath.org/ticket/7334#comment:13
https://trac.sagemath.org/ticket/7334#comment:13
<p>
started discussion <a class="ext-link" href="http://groups.google.cz/group/sage-devel/browse_thread/thread/3899a578da747009"><span class="icon"></span>on sage-devel</a>
</p>
Ticketrobert.marikFri, 13 Nov 2009 14:12:56 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:14
https://trac.sagemath.org/ticket/7334#comment:14
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
According to the discussion on sage-devel, let's wait (perhaps short time) and look for some cleaner solution.
</p>
TicketburcinWed, 03 Feb 2010 15:09:27 GMTstatus, author changed; reviewer, upstream set
https://trac.sagemath.org/ticket/7334#comment:15
https://trac.sagemath.org/ticket/7334#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Karl-Dieter Crisman</em>
</li>
<li><strong>upstream</strong>
set to <em>N/A</em>
</li>
<li><strong>author</strong>
changed from <em>whuss</em> to <em>Robert Mařík</em>
</li>
</ul>
<p>
3 months is more than enough time to wait. The patch looks good to me (apart from minor formatting problems like long lines). I want to give a positive review, but it seems I can't switch from needs_work to positive_review directly. I'll run doctests first, then come back here.
</p>
TicketburcinWed, 03 Feb 2010 15:24:00 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:16
https://trac.sagemath.org/ticket/7334#comment:16
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
On Sage-4.3.2.alpha1, I get these doctest failures:
</p>
<pre class="wiki">sage -t devel/sage-s/sage/symbolic/expression.pyx
**********************************************************************
File "/scratch/berocal/sage/i686/sage-4.3.rc0/devel/sage-s/sage/symbolic/expression.pyx", line 5837:
sage: (x*log(9)).simplify_log('all')
Expected:
log(9^x)
Got:
x*log(9)
**********************************************************************
File "/scratch/berocal/sage/i686/sage-4.3.rc0/devel/sage-s/sage/symbolic/expression.pyx", line 5849:
sage: (log(5)-log(2)).simplify_log()
Expected:
-log(2) + log(5)
Got:
log(5/2)
**********************************************************************
</pre><p>
I don't know about the first one, but the second one seems to be confirming a bug fix in Maxima. :)
</p>
Ticketrobert.marikWed, 03 Feb 2010 15:32:18 GMT
https://trac.sagemath.org/ticket/7334#comment:17
https://trac.sagemath.org/ticket/7334#comment:17
<p>
The first problem is outside maxima, since I have
</p>
<pre class="wiki">sage: maxima("logconcoeffp:'logconfun")
logconfun
sage: maxima("logconfun(m):= true")
logconfun(m):=true
sage: maxima("logcontract(x*log(9))")
log(9^x)
</pre><p>
I have to investigate the problem in details (perhaps tomorrow).
</p>
<p>
Yes, the second "problem" is a fixed bug from Maxima :)
</p>
Ticketrobert.marikWed, 03 Feb 2010 21:19:57 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7334-logcontract-5.patch</em>
</li>
</ul>
Ticketrobert.marikWed, 03 Feb 2010 21:24:00 GMT
https://trac.sagemath.org/ticket/7334#comment:18
https://trac.sagemath.org/ticket/7334#comment:18
<p>
New patch (switch temporary logexpand to false when doing logcontract) is attched. Apply only this patch.
</p>
<pre class="wiki">./sage -t devel/sage/sage/symbolic
</pre><p>
passed. Running all tests now. If they pass, I'll mark it as 'needs review' (tomorrow morning).
</p>
Ticketrobert.marikThu, 04 Feb 2010 07:37:26 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac-7334-logcontract-5-bugfix.patch</em>
</li>
</ul>
<p>
apply after trac-7334-logcontract-5.patch
</p>
Ticketrobert.marikThu, 04 Feb 2010 11:45:52 GMT
https://trac.sagemath.org/ticket/7334#comment:19
https://trac.sagemath.org/ticket/7334#comment:19
<p>
tests passed. apply patches trac-7334-logcontract-5.patch and trac-7334-logcontract-5-bugfix.patch
</p>
Ticketrobert.marikThu, 04 Feb 2010 12:15:23 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:20
https://trac.sagemath.org/ticket/7334#comment:20
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketkcrismanThu, 04 Feb 2010 16:05:47 GMTattachment set
https://trac.sagemath.org/ticket/7334
https://trac.sagemath.org/ticket/7334
<ul>
<li><strong>attachment</strong>
set to <em>trac_7334-logcontract-5-reviewer.patch</em>
</li>
</ul>
<p>
Apply after others
</p>
TicketkcrismanThu, 04 Feb 2010 16:08:44 GMT
https://trac.sagemath.org/ticket/7334#comment:21
https://trac.sagemath.org/ticket/7334#comment:21
<p>
Reviewer patch adds some additional doctests, fixes typos, clarifies a few other things. It also fixes a bug which may not have appeared on the author's platform, essentially the same one as in the 5-bugfix patch but for the log_expand function.
</p>
<p>
I don't really understand why the original code didn't work, because it should! But for some reason the logexpand variable was sticking around, also messing up other doctests in expression.pyx, for me, so I made this change. Only this change needs review now; all else is enthusiastic positive review!
</p>
TicketkcrismanThu, 04 Feb 2010 16:10:41 GMT
https://trac.sagemath.org/ticket/7334#comment:22
https://trac.sagemath.org/ticket/7334#comment:22
<p>
An additional thought on this:
</p>
<p>
And to be honest, we should reset logexpand anyway, because after using log_expand one might want to do something else with Maxima where one might want the default logexpand setting, and be surprised that logexpand had been changed by log_expand.
</p>
Ticketrobert.marikThu, 04 Feb 2010 19:01:49 GMT
https://trac.sagemath.org/ticket/7334#comment:23
https://trac.sagemath.org/ticket/7334#comment:23
<p>
The reviwer patch seems to be O.K. for me. Thanks for fixing typos and adding docs. Running tests now.
</p>
<p>
restoring default value of logexpand was not necessary, since 'ev' environment has been used in my original patch and this has no influence to the value of logexpand
</p>
<pre class="wiki">sage: maxima('logexpand')
true
sage: maxima('ev(log(x*y),logexpand:false)')
log(x*y)
sage: maxima('logexpand')
true
sage: maxima('ev(log(x*y),logexpand:super)')
log(y)+log(x)
sage: maxima('logexpand')
true
</pre><p>
but the current method without 'ev' enviroment is also O.K. and perhaps better, since ev may lead sometimes to some other problems and should be avoided if possible (as I understand discussions related (for example)to substitution from maxima group).
</p>
TicketkcrismanThu, 04 Feb 2010 20:59:15 GMT
https://trac.sagemath.org/ticket/7334#comment:24
https://trac.sagemath.org/ticket/7334#comment:24
<blockquote class="citation">
<p>
restoring default value of logexpand was not necessary, since 'ev' environment has been used in my original patch and this has no influence to the value of logexpand
</p>
</blockquote>
<p>
It shouldn't have, but for some reason it did in my installation - specifically in some tests in solve and friends which had logs in their answers that mysteriously began simplifying! Anyway, glad you think this solution is okay.
</p>
Ticketrobert.marikThu, 04 Feb 2010 21:20:52 GMT
https://trac.sagemath.org/ticket/7334#comment:25
https://trac.sagemath.org/ticket/7334#comment:25
<p>
all tests passed for me after trac-7334-logcontract-5.patch , trac-7334-logcontract-5-bugfix.patch, trac_7334-logcontract-5-reviewer.patch
</p>
TicketkcrismanFri, 05 Feb 2010 20:02:50 GMTstatus changed
https://trac.sagemath.org/ticket/7334#comment:26
https://trac.sagemath.org/ticket/7334#comment:26
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/7334#comment:25" title="Comment 25">robert.marik</a>:
</p>
<blockquote class="citation">
<p>
all tests passed for me after trac-7334-logcontract-5.patch , trac-7334-logcontract-5-bugfix.patch, trac_7334-logcontract-5-reviewer.patch
</p>
</blockquote>
<p>
It sounds like this means positive review for the last reviewer change. Great!
</p>
TicketmpatelThu, 11 Feb 2010 15:02:55 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/7334#comment:27
https://trac.sagemath.org/ticket/7334#comment:27
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.3.3.alpha0</em>
</li>
</ul>
Ticket