Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#17556 closed enhancement (fixed)

Move simplify_log() from simplify_full() to simplify_real()

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: 3616d00 (Commits) Commit:
Dependencies: Stopgaps:

Description

The simplify_log function assumes that its argument is real; otherwise the log contraction operation is invalid. For example,

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

Now that we have a simplify_real() method, why not move simplify_log() there instead?

In the process, the newer simplify_rectform() can be added to simplify_full().

I'm working on a patch for this.

Change History (12)

comment:1 Changed 5 years ago by mjo

  • Authors set to Michael Orlitzky
  • Branch set to u/mjo/ticket/17556
  • Commit set to 044db1d2b7bc4e914e92ed7aa87629cbe958bc52
  • Status changed from new to needs_review

Not too much had to be changed for this. Since the 'one' algorithm was used for simplify_log() within simplify_full(), it rarely had any effect.


New commits:

fbc0a40Trac #17556: Remove simplify_log() from simplify_full().
3ea1dedTrac #17556: Add simplify_rectform() to simplify_full().
044db1dTrac #17556: Add Expression.simplify_log() to Expression.simplify_real().

comment:2 Changed 5 years ago by rws

  • Reviewers set to Ralf Stephan
  • Status changed from needs_review to needs_work
sage -t --long src/doc/de/thematische_anleitungen/sage_gymnasium.rst  # 1 doctest failed

comment:3 Changed 5 years ago by mjo

I'm going to need some help from a native speaker to fix that. The context of that sentence is discussing the simplify_full() method:

Falls wir alle Terme so weit wie möglich vereinfachen möchten, erreichen wir dies mit der ``simplify_full()`` Funktion::

    sage: (sin(x)^2 + cos(x)^2).simplify_full()
    1

Then the failing test mentions (I think) that simplify_full() will also do something with the logarithms:

Dabei werden auch Additionstheoreme für trigonometrische Funktionen und manche Logarithmengesetze eingesetzt::

    sage: var('x, y, z')
    (x, y, z)
    sage: (sin(x + y)/(log(x) + log(y))).simplify_full()
    (cos(y)*sin(x) + cos(x)*sin(y))/log(x*y)
    sage: (sin(x)^2 + cos(x)^2).simplify_full()
    1

but of course that's not the case anymore because the log contraction isn't necessarily valid if x or y is complex (what's the z for???).

One possible solution would be to mention simplify_real() here. Note that the sin^2 + cos^2 == 1 identity is mentioned and tested twice in a row. Maybe we could drop the second one, mention something about simplify_real() being used when the variables are real, and then show an example of,

sage: (log(x) + log(y)).simplify_real()
log(x*y)

Another possible solution is to get rid of the log()s and just don't mention them. That takes slightly less creativity but does lose a useful example =)

comment:4 follow-ups: Changed 5 years ago by kcrisman

Probably something like this would be appropriate (Ralf, I assume your German is "more native" than mine and can confirm):

Mit der verwandten Funktion ``simplify_real()`` werden auch Additionstheoreme ...

and then use simplify_real() there.


That said, I'm not 100% convinced that simplify_full() should be "only the best" simplifications. One of the distinct advantages of that function is that it did pretty much everything you wanted. If we keep on changing what you need to do for various simplifications, it will become quite annoying to use Sage for such things in the classroom. (Not to mention making various books immediately invalid.)

comment:5 in reply to: ↑ 4 Changed 5 years ago by mjo

Replying to kcrisman:

That said, I'm not 100% convinced that simplify_full() should be "only the best" simplifications. One of the distinct advantages of that function is that it did pretty much everything you wanted. If we keep on changing what you need to do for various simplifications, it will become quite annoying to use Sage for such things in the classroom. (Not to mention making various books immediately invalid.)


I don't like removing them, but I do think we should try to avoid answers that are clearly wrong under the "simplify" name, like the example in the ticket description. We just don't have a lot of good simplifications available that work for complex variables. I did add simplify_rectform() to simplify_full() in this ticket as compensation.

In any case, this is a much smaller change than it might seem. I mention in the commit messages that the 'one' algorithm was passed to simplify_log() via simplify_full(), which means that log(x) + log(y) would be contracted to log(x*y) but 2*log(x) + log(y) wouldn't. Why that choice was made we'll never know, but the result is that the bug was pretty rare. A useful simplification was also exceedingly rare.

I think the change only affected two doctests (including the German one), and in those cases it's not clear that the variables were supposed to be real, so the answers may be wrong anyway =)

With simplify_log() moved into simplify_real(), I've made it use the default algorithm. So now 2*log(x) + log(y) will be simplified to log(x^2*y) as well. That makes this particular simplification much more useful to people who use it. Before you would have had to dig into the simplify_full() and simplify_log() source to figure out a) why you weren't getting it, and, b) how to make it happen (with a separate call to simplify_log()).

So tl;dr I felt bad removing two things from simplify_full() in a row but this one nobody will notice. And I added one back!

comment:6 Changed 5 years ago by rws

  • Branch changed from u/mjo/ticket/17556 to u/rws/ticket/17556

comment:7 in reply to: ↑ 4 Changed 5 years ago by rws

  • Commit changed from 044db1d2b7bc4e914e92ed7aa87629cbe958bc52 to 3616d00f582e175005d9b0182a1c1f4c124ebb56
  • Status changed from needs_work to positive_review

Oops, should have used a public branch.

Replying to kcrisman:

Probably something like this would be appropriate (Ralf, I assume your German is "more native" than mine and can confirm):

Mit der verwandten Funktion ``simplify_real()`` werden auch Additionstheoreme ...

and then use simplify_real() there.

This patch adds such a test, and the sentence reads translated: With the related function simplify_real() also logarithmic addition theorems are applied, which hold for real values.

Since it's practically what you both proposed I'm including it with my positive review.


New commits:

3616d0017556: reviewer's patch: adapt text and test to changed behaviour

comment:8 Changed 5 years ago by kcrisman

The "die nur mit reellen Werten erlaubt sind" is perfect, great!

comment:9 Changed 5 years ago by SimonKing

Sorry to be so late. I think the following does not sound optimal:

Mit der verwandten Funktion ``simplify_real()`` werden auch Additionstheoreme
bei Logarithmen angewandt, die nur mit reellen Werten erlaubt sind::

I think in "Additionstheoreme bei Logarithmen", the "bei" is wrong, and also I wouldn't say that a theorem is "erlaubt".

What do you think about "werden auch Additionstheoreme verwendet, die nur für reellwertige Logarithmen gelten"? Or at least "werden auch solche Additionstheoreme für Logarithmen angewandt, die nur mit reellen Werten gelten"? (or "anwendbar sind" instead of "gelten")

comment:10 Changed 5 years ago by SimonKing

Do I understand correctly that the purpose of this ticket is not to fix further errors in sage_gymnasium? I just noticed that the example in the section on "Partialbruchzerlegung" gives a wrong result.

comment:11 Changed 5 years ago by vbraun

  • Branch changed from u/rws/ticket/17556 to 3616d00f582e175005d9b0182a1c1f4c124ebb56
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 5 years ago by kcrisman

  • Commit 3616d00f582e175005d9b0182a1c1f4c124ebb56 deleted

How about Additionstheoreme, deren Benutzung nur mit reellen Werten erlaubt sind or gelten? But I didn't learn my math in German - deren Hypothesen nur für reelen Werte ...? Anyway, Simon, definitely feel free to open another ticket to make this better as well as fixing anything else in sage_gymnasium.

Last edited 5 years ago by kcrisman (previous) (diff)
Note: See TracTickets for help on using tickets.