Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

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

Reported by: Owned by: Michael Orlitzky major sage-6.5 symbolics Michael Orlitzky Ralf Stephan N/A 3616d00

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

### comment:1 Changed 8 years ago by Michael Orlitzky

Authors: → Michael Orlitzky → u/mjo/ticket/17556 → 044db1d2b7bc4e914e92ed7aa87629cbe958bc52 new → 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:

 ​fbc0a40 `Trac #17556: Remove simplify_log() from simplify_full().` ​3ea1ded `Trac #17556: Add simplify_rectform() to simplify_full().` ​044db1d `Trac #17556: Add Expression.simplify_log() to Expression.simplify_real().`

### comment:2 Changed 8 years ago by Ralf Stephan

Reviewers: → Ralf Stephan needs_review → needs_work
```sage -t --long src/doc/de/thematische_anleitungen/sage_gymnasium.rst  # 1 doctest failed
```

### comment:3 Changed 8 years ago by Michael Orlitzky

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:  5  7 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Michael Orlitzky

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 8 years ago by Ralf Stephan

Branch: u/mjo/ticket/17556 → u/rws/ticket/17556

### comment:7 in reply to:  4 Changed 8 years ago by Ralf Stephan

Commit: 044db1d2b7bc4e914e92ed7aa87629cbe958bc52 → 3616d00f582e175005d9b0182a1c1f4c124ebb56 needs_work → 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:

 ​3616d00 `17556: reviewer's patch: adapt text and test to changed behaviour`

### comment:8 Changed 8 years ago by Karl-Dieter Crisman

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

### comment:9 Changed 8 years ago by Simon King

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 8 years ago by Simon King

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 8 years ago by Volker Braun

Branch: u/rws/ticket/17556 → 3616d00f582e175005d9b0182a1c1f4c124ebb56 → fixed positive_review → closed

### comment:12 Changed 8 years ago by Karl-Dieter Crisman

Commit: 3616d00f582e175005d9b0182a1c1f4c124ebb56

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 8 years ago by Karl-Dieter Crisman (previous) (diff)
Note: See TracTickets for help on using tickets.