2607: more efficient implementation of Add.as_numer_denom

    By collecting denominators and processing Integer denominators,
    as_numer_denom is more efficient. When denominators are collected
    this produces less terms to process; when Integers are handled
    separately, their denominator can be computed very quickly with
    integer arithmetic.

    Because of collection of denominators, there will be a slight
    bit of nesting that will occur:

    >>> (1/x+a/x+b/y).as_numer_denom()
    (b*x + y*(a + 1), x*y)

    Processing Integer denominators will not result in nesting as
    a result of the automatic distribution of Rationals on Adds:

    >>> (a/1+b/2+c/3+d/4).as_numer_denom()
    (12*a + 6*b + 4*c + 3*d, 12)

    (Note, too, that the numerical gcd of numerator and denominator
    has been removed so (24*a + 12*b + 8*c + 6*d, 24) is not obtained.

    There are other possibilities for improvement: if one doesn't care
    about producing a nested numerator, a binary approach can be employed
    where pairs of terms are processed. If a mostly cancelled expression
    is desired, a symbolic lcm method (Vladimir's approach) can be used.

    One of the dilemmas is to know when to use one method over another;
    and testing to see which method to use can eliminate the savings that
    a technique might have over another. Testing for common denominators
    and integers are a fairly easy preprocessing step that can lead to
    large improvements over a more naive method.

    This work was a collaborative effort of Aaron Meurer, Vladimir Peric,
    and Christopher Smith.

sympy/core/add.py

@@ -4,7 +4,9 @@
 from operations import AssocOp
 from cache import cacheit
 from expr import Expr
-from numbers import ilcm
+from numbers import ilcm, igcd
+
+from collections import defaultdict
 
 class Add(AssocOp):
 
@@ -283,13 +285,50 @@ def as_two_terms(self):
         return self.args[0], self._new_rawargs(*self.args[1:])
 
     def as_numer_denom(self):
-        numers, denoms = [],[]
-        for n,d in [f.as_numer_denom() for f in self.args]:
+        # collect numerators and denominators of the terms
+        nd = defaultdict(list)
+        for f in self.args:
+            ni, di = f.as_numer_denom()
+            nd[di].append(ni)
+
+        # check for quick exit
+        if len(nd) == 1:
+            d, n = nd.popitem()
+            return Add(*n), d
+
+        # separate out those that are Integers and add those that
+        # had a common denominator
+        nnum = []
+        dnum = []
+        for d, n in nd.items():
+            nd[d] = Add(*nd[d])
+            if d.is_Integer:
+                nnum.append(nd.pop(d))
+                dnum.append(int(d))
+
+        # prepare list of those terms that didn't have integer denoms
+        if nd:
+            denoms, numers = [list(i) for i in zip(*nd.iteritems())]
+        else:
+            numers = []
+            denoms = []
+
+        # process the Number denominators efficiently
+        if nnum:
+            den = reduce(ilcm, dnum, 1)
+            resid = [den/di for di in dnum]
+            n = Add(*[resid[i]*ni for i, ni in enumerate(nnum)])
             numers.append(n)
-            denoms.append(d)
-        r = xrange(len(numers))
-        return Add(*[Mul(*(denoms[:i]+[numers[i]]+denoms[i+1:]))
-                     for i in r]), Mul(*denoms)
+            denoms.append(S(den))
+
+        if len(numers) == 1:
+            n, d = numers[0], denoms[0]
+
+        else:
+            n, d = Add(*[Mul(*(denoms[:i]+[numers[i]]+denoms[i+1:]))
+                       for i in xrange(len(numers))]), Mul(*denoms)
+
+        return n, d
 
     def _eval_is_polynomial(self, syms):
         return all(term._eval_is_polynomial(syms) for term in self.args)

sympy/polys/tests/test_polyroots.py

@@ -263,14 +263,15 @@ def test_roots():
 
     f = (x**2+2*x+3).subs(x, 2*x**2 + 3*x).subs(x, 5*x-4)
 
-    r1_2, r13_20, r1_100 = [ Rational(*r) \
-        for r in ((1,2), (13,20), (1,100)) ]
+    r13_20, r1_20 = [ Rational(*r) \
+        for r in ((13,20), (1,20)) ]
 
+    s2 = sqrt(2)
     assert roots(f, x) == {
-        r13_20 + r1_100*(25 - 200*I*2**r1_2)**r1_2: 1,
-        r13_20 - r1_100*(25 - 200*I*2**r1_2)**r1_2: 1,
-        r13_20 + r1_100*(25 + 200*I*2**r1_2)**r1_2: 1,
-        r13_20 - r1_100*(25 + 200*I*2**r1_2)**r1_2: 1,
+        r13_20 + r1_20*sqrt(1 - 8*I*s2): 1,
+        r13_20 - r1_20*sqrt(1 - 8*I*s2): 1,
+        r13_20 + r1_20*sqrt(1 + 8*I*s2): 1,
+        r13_20 - r1_20*sqrt(1 + 8*I*s2): 1,
     }
 
     f = x**4 + x**3 + x**2 + x + 1

sympy/solvers/solvers.py

@@ -1202,7 +1202,7 @@ def solve_linear(lhs, rhs=0, symbols=[], exclude=[]):
     expression without causing a division by zero error.
 
         >>> solve_linear(x**2*(1/x - z**2/x))
-        (x**2*(-x*z**2 + x), x**2)
+        (x**2*(-z**2 + 1), x)
 
     You can give a list of what you prefer for x candidates: