Opened 2 years ago
Closed 2 years ago
#27304 closed defect (fixed)
Bug in factorization of simple symbolic expressions
Reported by:  egourgoulhon  Owned by:  

Priority:  blocker  Milestone:  sage8.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, GitHub, GitLab)  Commit:  7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52 
Dependencies:  Stopgaps: 
Description (last modified by )
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/sagedevel/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 2 years ago by
 Description modified (diff)
comment:2 Changed 2 years ago by
 Priority changed from major to critical
comment:3 followup: ↓ 5 Changed 2 years ago by
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
 Keywords pynac added
comment:5 in reply to: ↑ 3 Changed 2 years ago by
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
 Description modified (diff)
comment:7 Changed 2 years ago by
This sagesupport post might be related...
comment:8 Changed 2 years ago by
 Milestone changed from sage8.7 to sage8.8
Moving all blocker/critical issues from 8.7 to 8.8.
comment:9 Changed 2 years ago by
 Priority changed from critical to blocker
comment:10 Changed 2 years ago by
 Cc bpage added
comment:11 Changed 2 years ago by
 Description modified (diff)
comment:12 followup: ↓ 13 Changed 2 years ago by
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 ; followup: ↓ 14 Changed 2 years ago by
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 ; followup: ↓ 15 Changed 2 years ago by
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 ; followup: ↓ 16 Changed 2 years ago by
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 (shortterm) bugfix I proposed above.
comment:16 in reply to: ↑ 15 Changed 2 years ago by
Replying to behackl:
I already thought a bit about it, so yes, I will prepare a branch with the (shortterm) bugfix I proposed above.
Great!
comment:17 Changed 2 years ago by
 Branch set to u/behackl/simplefactorizationrational
 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 nonrational expressions

7c2432c  fix doctest; add more tests

comment:18 Changed 2 years ago by
 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 warnlong 151.5 src/sage/tests/books/computationalmathematicswithsagemath/graphique_doctest.py # 1 doctest failed 
which passes when ran standalone.
==> positive_review
.
comment:19 Changed 2 years ago by
Thank you Benjamin!
comment:20 Changed 2 years ago by
 Branch changed from u/behackl/simplefactorizationrational to 7c2432ce2fdfe7eb905b91179a0fcaccef3d0a52
 Resolution set to fixed
 Status changed from positive_review to closed
in the same vein :
which is true, but largely not what we seek...
which is false. But :
Deserves to be
critical
, if notblocker
.