Opened 5 months ago

Closed 2 months ago

#27304 closed defect (fixed)

Bug in factorization of simple symbolic expressions

Reported by: egourgoulhon Owned by:
Priority: blocker Milestone: sage-8.8
Component: symbolics Keywords: factor, exponential, pynac
Cc: rws, bpage Merged in:
Authors: Benjamin Hackl Reviewers: Emmanuel Charpentier
Report Upstream: N/A Work issues:
Branch: 7c2432c (Commits) Commit: 7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52
Dependencies: Stopgaps:

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

Change History (20)

comment:1 Changed 5 months ago by egourgoulhon

  • Description modified (diff)

comment:2 Changed 5 months 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: Changed 5 months 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 5 months ago by egourgoulhon

  • Keywords pynac added

comment:5 in reply to: ↑ 3 Changed 5 months 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 5 months ago by egourgoulhon

  • Description modified (diff)

comment:7 Changed 5 months ago by charpent

This sage-support post might be related...

comment:8 Changed 4 months 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 3 months ago by slelievre

  • Priority changed from critical to blocker

comment:10 Changed 3 months ago by bpage

  • Cc bpage added

comment:11 Changed 2 months ago by egourgoulhon

  • Description modified (diff)

comment:12 follow-up: Changed 2 months 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: Changed 2 months 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: Changed 2 months 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: Changed 2 months 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 months 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 months 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:

0618fd3helper method: is_rational_expression
131cd84let maxima handle factorization of non-rational expressions
7c2432cfix doctest; add more tests
Last edited 2 months ago by behackl (previous) (diff)

comment:18 Changed 2 months 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 months ago by charpent (previous) (diff)

comment:19 Changed 2 months ago by egourgoulhon

Thank you Benjamin!

comment:20 Changed 2 months 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.