Opened 2 years ago

Closed 2 years ago

# Bug in factorization of simple symbolic expressions

Reported by: Owned by: egourgoulhon blocker sage-8.8 symbolics factor, exponential, pynac rws, bpage Benjamin Hackl Emmanuel Charpentier N/A 7c2432c 7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52

### Description (last modified by egourgoulhon)

As reported in this ask.sagemath question, we have currently (Sage 8.7.beta3):

```sage: factor(2*exp(x) + exp(-x))
3*e^x
```

as well as

```sage: factor(x*exp(-x) + exp(-x))
(x + 1)*e^x
```

Another example of erroneous result exhibited in https://groups.google.com/d/msg/sage-devel/ytLqIb4soLw/c14ZKGqcAAAJ is

```sage: var('t u')
(t, u)
sage: L = (u + t)/(u - t)
sage: factor(L.substitute(t=sqrt(u)))
(u + 1)/(u - 1)
```

This bug is there since at least Sage 8.4.beta4. It is not in Sage 8.3, hence it must have been introduced between 8.4.beta0 and 8.4.beta4. It seems to have been introduced in #23835 (see comment:5).

### comment:1 Changed 2 years ago by egourgoulhon

• Description modified (diff)

### comment:2 Changed 2 years ago by charpent

• Priority changed from major to critical

in the same vein :

```sage: e^(x+2*I*pi)
cosh(-x) - sinh(-x)
```

which is true, but largely not what we seek...

```sage: e^(x+2*I*pi)==e^x
cosh(-x) - sinh(-x) == e^x
sage: bool(e^(x+2*I*pi)==e^x)
False
```

which is false. But :

```sage: e^(x+2*I*pi).maxima_methods().exponentialize()
cosh(-x) - sinh(-x)
sage: (e^(x+2*I*pi)).maxima_methods().exponentialize()
e^x
```

Deserves to be `critical`, if not `blocker`.

### comment:3 follow-up: ↓ 5 Changed 2 years ago by egourgoulhon

A difference between Sage 8.3 (where the factorization works) and Sage 8.4.beta4 is that in Sage 8.3 the factorization is performed by Maxima: cf. these lines of `src/sage/symbolic/expression.pyx`

```    def factor(self, dontfactor=[]):
from sage.calculus.calculus import symbolic_expression_from_maxima_string, symbolic_expression_from_string
if len(dontfactor) > 0:
m = self._maxima_()
name = m.name()
varstr = ','.join(['_SAGE_VAR_'+str(v) for v in dontfactor])
cmd = 'block([dontfactor:[%s]],factor(%s))'%(varstr, name)
return symbolic_expression_from_maxima_string(cmd)
else:
try:
from sage.rings.all import QQ
f = self.polynomial(QQ)
w = repr(f.factor())
return symbolic_expression_from_string(w)
except (TypeError, NotImplementedError):
pass
return self.parent()(self._maxima_().factor())

```

while in Sage 8.4.beta4 and later, it is performed by pynac (via the function `g_factor`):

```     def factor(self, dontfactor=[]):
from sage.calculus.calculus import symbolic_expression_from_maxima_string
cdef GEx x
cdef bint b
if dontfactor:
m = self._maxima_()
name = m.name()
varstr = ','.join(['_SAGE_VAR_' + str(v) for v in dontfactor])
cmd = 'block([dontfactor:[%s]],factor(%s))' % (varstr, name)
return symbolic_expression_from_maxima_string(cmd)
sig_on()
try:
b = g_factor(self._gobj, x)
finally:
sig_off()
if b:
return new_Expression_from_GEx(self._parent, x)
else:
return self
```

Maxima is correct there: in Sage 8.7.beta3, we have

```./sage -maxima
(%i1) factor(2*exp(x) + exp(-x));
- x      2 x
(%o1)                         %e    (2 %e    + 1)
```

So I would say it is a pynac bug.

### comment:4 Changed 2 years ago by egourgoulhon

• Keywords pynac added

### comment:5 in reply to: ↑ 3 Changed 2 years ago by egourgoulhon

The change from Maxima factorization to Pynac one, which triggered the bug, was introduced in #23835, which has been merged in Sage 8.4.beta3.

### comment:6 Changed 2 years ago by egourgoulhon

• Description modified (diff)

### comment:7 Changed 2 years ago by charpent

This sage-support post might be related...

### comment:8 Changed 2 years ago by embray

• Milestone changed from sage-8.7 to sage-8.8

Moving all blocker/critical issues from 8.7 to 8.8.

### comment:9 Changed 2 years ago by slelievre

• Priority changed from critical to blocker

### comment:10 Changed 2 years ago by bpage

• Cc bpage added

### comment:11 Changed 2 years ago by egourgoulhon

• Description modified (diff)

### comment:12 follow-up: ↓ 13 Changed 2 years ago by behackl

Would it be an option to let pynac handle factorization only when called on a rational function and call maxima (just as it was before #23835) otherwise?

### comment:13 in reply to: ↑ 12 ; follow-up: ↓ 14 Changed 2 years ago by charpent

Replying to behackl:

Would it be an option to let pynac handle factorization only when called on a rational function and call maxima (just as it was before #23835) otherwise?

Seconded :

If that means reverting the changes brought in by #23835, and if that does not break other changes intervening since, this would be a reasonable option (even if bad for performance), and would give people who know what they're talking about time to poner our next move...

### comment:14 in reply to: ↑ 13 ; follow-up: ↓ 15 Changed 2 years ago by egourgoulhon

Replying to charpent:

Replying to behackl:

Would it be an option to let pynac handle factorization only when called on a rational function and call maxima (just as it was before #23835) otherwise?

Seconded :

Idem, +1. This sounds a good strategy for the short term. It would indeed be a pity if Sage 8.8 is shipped with such a bug. Benjamin, do you intend to take this in charge?

### comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 2 years ago by behackl

Replying to egourgoulhon:

Idem, +1. This sounds a good strategy for the short term. It would indeed be a pity if Sage 8.8 is shipped with such a bug. Benjamin, do you intend to take this in charge?

I already thought a bit about it, so yes, I will prepare a branch with the (short-term) bugfix I proposed above.

### comment:16 in reply to: ↑ 15 Changed 2 years ago by egourgoulhon

Replying to behackl:

I already thought a bit about it, so yes, I will prepare a branch with the (short-term) bugfix I proposed above.

Great!

### comment:17 Changed 2 years ago by behackl

• Authors set to Benjamin Hackl
• Branch set to u/behackl/simple-factorization-rational
• Commit set to 7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52
• Status changed from new to needs_review

This is what I would suggest. I'm entirely open for suggestions regarding the name as well as the code for `is_rational_expression`.

Locally, I only checked the doctests in `expression.pyx`, maybe a patchbot finds further occurrences of the symbolic `factor` method where the output has to be changed.

New commits:

 ​0618fd3 `helper method: is_rational_expression` ​131cd84 `let maxima handle factorization of non-rational expressions` ​7c2432c `fix doctest; add more tests`
Last edited 2 years ago by behackl (previous) (diff)

### comment:18 Changed 2 years ago by charpent

• Reviewers set to Emmanuel Charpentier
• Status changed from needs_review to positive_review

On top of 8.8.beta5+#27738, builds and passes `ptestlong` exept for a transient (and already reported twice):

```----------------------------------------------------------------------
sage -t --long --warn-long 151.5 src/sage/tests/books/computational-mathematics-with-sagemath/graphique_doctest.py  # 1 doctest failed
----------------------------------------------------------------------
```

which passes when ran standalone.

==> `positive_review`.

Last edited 2 years ago by charpent (previous) (diff)

### comment:19 Changed 2 years ago by egourgoulhon

Thank you Benjamin!

### comment:20 Changed 2 years ago by vbraun

• Branch changed from u/behackl/simple-factorization-rational to 7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.