Opened 5 years ago

Closed 5 years ago

#17393 closed defect (fixed)

Add warning to Expression.simplify_log()

Reported by: mjo Owned by:
Priority: major Milestone: sage-6.5
Component: symbolics Keywords:
Cc: Merged in:
Authors: Michael Orlitzky Reviewers: Ralf Stephan
Report Upstream: N/A Work issues:
Branch: 7a74601 (Commits) Commit: 7a746013c05484d82362b5dc865f18db9681dde8
Dependencies: Stopgaps:

Description

The simplify_log() function should only be called when the expression is real, and I just noticed this isn't documented. For an example, take:

sage: x,y = SR.var('x,y')
sage: f = log(x*y) - (log(x) + log(y))
sage: f(x=-1, y=i)
-2*I*pi
sage: f.simplify_log()
0

The documentation does say what it will do, so if you know that the real log identities don't hold over the complex numbers, then it's documented. But I think it's better to warn explicitly.

Change History (7)

comment:1 Changed 5 years ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/17393
  • Commit set to 453e74c78803f062cd298bcad0cc5b4b5f2bf0b2
  • Status changed from new to needs_review

I cleaned up a little, too. A detailed description is in the commit message.


New commits:

453e74cTrac #17393: Add a domain warning to Expression.simplify_log().

comment:2 Changed 5 years ago by rws

  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to positive_review

Looks fine in the HTML.

comment:3 Changed 5 years ago by vbraun

  • Status changed from positive_review to needs_work
sage -t --long src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 8835, in sage.symbolic.expression.Expression.simplify_log
Failed example:
    f.simplify_log('one')
Expected:
    1/2*log(t) + log(x) + 2*log(y)
Got:
    -pi*log(x + 1) + log(x*y)
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 8838, in sage.symbolic.expression.Expression.simplify_log
Failed example:
    f.simplify_log('ratios')
Expected:
    log(sqrt(t)*x*y^2)
Got:
    -pi*log(x + 1) + log(x*y)
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 8841, in sage.symbolic.expression.Expression.simplify_log
Failed example:
    f.simplify_log()
Expected:
    log(x*y^2) + 1/2*log(t)
Got:
    -pi*log(x + 1) + log(x*y)
**********************************************************************
1 item had failures:
   3 of  31 in sage.symbolic.expression.Expression.simplify_log
    [2265 tests, 3 failures, 21.05 s]
----------------------------------------------------------------------
sage -t --long src/sage/symbolic/expression.pyx  # 3 doctests failed

comment:4 Changed 5 years ago by git

  • Commit changed from 453e74c78803f062cd298bcad0cc5b4b5f2bf0b2 to 7a746013c05484d82362b5dc865f18db9681dde8

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

e692893Trac #17393: Add a domain warning to Expression.simplify_log().
7a74601Trac #17393: Fix doctest failure introduced in previous commit.

comment:5 Changed 5 years ago by mjo

  • Status changed from needs_work to needs_review

Durr sorry. I must've forgotten to run sage -b before doctesting. I moved a block of tests but forgot to redefine the function f they were testing.

I rebased my branch on top of develop, and added one more line that should fix it.

comment:6 Changed 5 years ago by rws

  • Status changed from needs_review to positive_review

File passes tests. Docs compile, look good.

comment:7 Changed 5 years ago by vbraun

  • Branch changed from u/mjo/ticket/17393 to 7a746013c05484d82362b5dc865f18db9681dde8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.