Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

linsolve: added a check to remove `Prefix` objects from equation #14865

Merged
merged 10 commits into from Jul 17, 2018

Conversation

Projects
None yet
3 participants
@jashan498
Copy link
Contributor

jashan498 commented Jul 3, 2018

Fixes #14860

Brief description of what is fixed or changed

linsolve convert equations of the system to Poly object here. This makes expr.is_commutative False here, hence giving the error. I just added a check to quantity_simplify such equations.

Other comments

This PR also solves another bug, in addition to issue mentioned above:

>>> from sympy.physics.units import meter, newton, kilo
>>> from sympy.physics.units.util import quantity_simplify
>>> var('x y')
(x, y)
>>> quantity_simplify(x*(8*kilo*newton + y))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jashan/sympy/sympy/physics/units/util.py", line 148, in quantity_simplify
    coeff = coeff * arg.scale_factor
AttributeError: 'Add' object has no attribute 'scale_factor'

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 5, 2018

Alternatively,

diff --git a/sympy/physics/units/prefixes.py b/sympy/physics/units/prefixes.py
index 184c6d4..42f1500 100644
--- a/sympy/physics/units/prefixes.py
+++ b/sympy/physics/units/prefixes.py
@@ -29,6 +29,7 @@ class Prefix(Expr):
       class).
     """
     _op_priority = 13.0
+    is_commutative = True

     def __new__(cls, name, abbrev, exponent, base=sympify(10)):

diff --git a/sympy/physics/units/util.py b/sympy/physics/units/util.py
index 2af04bf..c15b13a 100644
--- a/sympy/physics/units/util.py
+++ b/sympy/physics/units/util.py
@@ -133,12 +133,13 @@ def get_total_scale_factor(expr):


 def quantity_simplify(expr):
-    if expr.is_Atom:
+    if expr.is_Atom or not expr.has(Quantity):
+        return expr
+    expr = expr.func(*map(quantity_simplify, expr.args))
+    if not isinstance(expr, Mul):
         return expr
-    if not expr.is_Mul:
-        return expr.func(*map(quantity_simplify, expr.args))

-    if expr.has(Prefix):
+    if any(isinstance(a, Prefix) for a in expr.args):
         coeff, args = expr.as_coeff_mul(Prefix)
         args = list(args)
         for arg in args:
diff --git a/sympy/solvers/solveset.py b/sympy/solvers/solveset.py
index 4ae1849..34f42d7 100644
--- a/sympy/solvers/solveset.py
+++ b/sympy/solvers/solveset.py
@@ -1516,14 +1516,13 @@ def linsolve(system, *symbols):
                     be given as a sequence, too.
                 '''))
             system = list(system)
+            from sympy.core.function import _mexpand
             for i, eq in enumerate(system):
                 try:
-                    # since we are checking it, we might as well take the
-                    # expanded expr that it will give
-                    system[i] = Poly(eq, symbols).as_expr()
-                    if any (degree(eq, sym) > 1 for sym in symbols):
-                        raise PolynomialError
-                except PolynomialError:
+                    # use fully expanded form of expr
+                    system[i] = _mexpand(eq, recursive=True)
+                    assert all(degree(eq, sym) <= 1 for sym in symbols)
+                except AssertionError:
                     raise ValueError(filldedent('''
                         %s contains non-linear terms in the
                         variables to be evaluated

Running tests now. This also allows the following to succeed:

>>> from sympy.physics.units import meter, newton, kilo
>>> from sympy.physics.units.util import quantity_simplify
>>> quantity_simplify(x*(8*kilo*newton + y))
x*(8000*newton + y)
>>> eq=8*kilo*newton + x + y
>>> solve(eq, x)
[-8000*newton - y]
@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 5, 2018

@smichr thanks, that diff was helpful. I removed all changes from solveset.linsolve as replacing Poly with _mexpand was skipping PolynomialError in case of linsolve([cos(x) + y, x + y], [x, y]) and gave wrong solution.

@jashan498 jashan498 force-pushed the jashan498:linsolve_prefix branch from df22803 to f44418f Jul 5, 2018

@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 5, 2018

I Found another bug(both in master and in this branch):

>>> quantity_simplify(x*(8*newton + y))
x*(8*newton + y, 1)

It may be because of expr.as_coeff_mul(Quantity) returning x and (8*newton+y) . I was thinking of doing expr = expr.expand() at the start of the method but it will destroy original type of the object passed.

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 7, 2018

something like this works:

def quantity_simplify(expr):
    from sympy import ordered
    # replace all prefixes with numbers
    nopre = expr.xreplace(dict([(i, i.scale_factor) for i in expr.atoms(Prefix)]))
    # reduce all mixed units of a given dimension to a single Quantity
    # e.g. foot*inch**2 -> 1/144*foot**3
    reps = {}
    for m in nopre.atoms(Mul):
        be = [i.as_base_exp() for i in m.args]
        be, c = sift(be, lambda x: isinstance(x[0], Quantity), binary=True)
        c = Mul(*[b**e for b, e in c])
        be = [(b.dimension, (b, e)) for b, e in be]
        s = {}
        for d, (b, e) in be:
            s.setdefault(d, []).append((b,e))
        f = []
        for k in s:
            s[k] = list(ordered(s[k]))
            B, e = s[k][0]
            if len(s[k]) > 1:
                scale = B.scale_factor
                f.append(Mul(*[(B*b.scale_factor/scale)**e for b, e in s[k]]))
            else:
                f.append(B**e)
        reps[m] = c*Mul(*f)
    return expr.subs(reps)
>>> from sympy.physics.units import *
>>> from sympy.physics.units.util import *
>>> f=quantity_simplify

>>> f(foot*inch**2*centimeter)
6145149*centimeter**4/31250

>>> f(x*(8*newton*meter+y))
x*(8*meter*newton + y)
@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 7, 2018

After doing Muls, Adds should be done to simplify 3foot + 6inch into 3.5foot or 42inch.

@jashan498 jashan498 force-pushed the jashan498:linsolve_prefix branch from 58abc06 to 4df5996 Jul 8, 2018

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 8, 2018

It doesn't really look like that quantity_simplify is even needed with the other changes that have been made. But what about doing something like this:

diff --git a/sympy/physics/units/tests/test_util.py b/sympy/physics/units/tests/test_util.py
index 7394706..3ede418 100644
--- a/sympy/physics/units/tests/test_util.py
+++ b/sympy/physics/units/tests/test_util.py
@@ -6,7 +6,7 @@
 
 from sympy.utilities.exceptions import SymPyDeprecationWarning
 
-from sympy import Add, Mul, Pow, Tuple, pi, sin, sqrt, sstr, sympify
+from sympy import Add, Mul, Pow, Tuple, pi, sin, sqrt, sstr, sympify, oo
 from sympy.physics.units import (
     G, centimeter, coulomb, day, degree, gram, hbar, hour, inch, joule, kelvin,
     kilogram, kilometer, length, meter, mile, minute, newton, planck,
@@ -115,7 +115,8 @@ def test_convert_to_tuples_of_quantities():
 
 
 def test_eval_simplify():
-    from sympy.physics.units import cm, mm, km, m, K, Quantity, kilo
+    from sympy.physics.units import (cm, mm, km, m, K,
+        Quantity, kilo, foot, inch)
     from sympy.simplify.simplify import simplify
     from sympy.core.symbol import symbols
     from sympy.utilities.pytest import raises
@@ -127,8 +128,11 @@ def test_eval_simplify():
     assert ((km/m).simplify()) == 1000
     assert ((km/cm).simplify()) == 100000
     assert ((10*x*K*km**2/m/cm).simplify()) == 1000000000*x*kelvin
-    assert ((cm/km/m).simplify()) == 1/(100*kilometer)
+    assert ((cm/km/m).simplify()) == 1/(100000*meter)
 
     assert (3*kilo*meter).simplify() == 3000*meter
     assert (4*kilo*meter/(2*kilometer)).simplify() == 2
     assert (4*kilometer**2/(kilo*meter)**2).simplify() == 4
+
+    assert (foot + inch).simplify(oo) == 1651*meter/5000
+    assert (foot*inch).simplify(oo) == 48387*meter**2/6250000
diff --git a/sympy/physics/units/util.py b/sympy/physics/units/util.py
index c15b13a..eff5d61 100644
--- a/sympy/physics/units/util.py
+++ b/sympy/physics/units/util.py
@@ -133,31 +133,36 @@ def get_total_scale_factor(expr):
 
 
 def quantity_simplify(expr):
-    if expr.is_Atom or not expr.has(Quantity):
-        return expr
-    expr = expr.func(*map(quantity_simplify, expr.args))
-    if not isinstance(expr, Mul):
-        return expr
+    # XXX make this do simplifications like converting
+    # joule/coulomb to volt
+    return SI_units(expr)
+
+
+def SI_units(expr):
+    from sympy.physics.units.definitions import (percent, permille,
+        radian, degree, angular_mil, avogadro_number,
+        meter, liter, gram, second, ampere, kelvin, mole, candela,
+        hertz, coulomb, dioptre, lux, katal, gray, becquerel, bit)
+    # replace all prefixes with numbers
+    rv = expr.xreplace(dict([(i, i.scale_factor)
+        for i in expr.atoms(Prefix)]))
+    # convert all Quanities to base quanitities
+    rv = rv.xreplace({percent: percent.scale_factor})
+    rv = rv.xreplace({permille: permille.scale_factor})
+    rv = rv.xreplace({degree: degree.convert_to(radian)})
+    rv = rv.xreplace({angular_mil: degree.convert_to(radian)})
+    rv = rv.xreplace({avogadro_number: avogadro_number.scale_factor})
+    # steradian unchanged
+    q = rv.atoms(Quantity)
+    reps = {}
+    for i in q:
+        for b in [meter, liter, gram, second, ampere, kelvin, mole,
+                candela, hertz, coulomb, dioptre, lux, katal, gray,
+                becquerel, bit]:
+            if i.dimension == b.dimension:
+                reps[i] = i.convert_to(b)
+                break
+    rv = rv.xreplace(reps)
+    rv = rv.xreplace({liter: liter.convert_to(meter)})
+    return rv
 
-    if any(isinstance(a, Prefix) for a in expr.args):
-        coeff, args = expr.as_coeff_mul(Prefix)
-        args = list(args)
-        for arg in args:
-            if isinstance(arg, Pow):
-                coeff = coeff * (arg.base.scale_factor ** arg.exp)
-            else:
-                coeff = coeff * arg.scale_factor
-        expr = coeff
-
-    coeff, args = expr.as_coeff_mul(Quantity)
-    args_pow = [arg.as_base_exp() for arg in args]
-    quantity_pow, other_pow = sift(args_pow, lambda x: isinstance(x[0], Quantity), binary=True)
-    quantity_pow_by_dim = sift(quantity_pow, lambda x: x[0].dimension)
-    # Just pick the first quantity:
-    ref_quantities = [i[0][0] for i in quantity_pow_by_dim.values()]
-    new_quantities = [
-        Mul.fromiter(
-            (quantity*i.scale_factor/quantity.scale_factor)**p for i, p in v)
-            if len(v) > 1 else v[0][0]**v[0][1]
-        for quantity, (k, v) in zip(ref_quantities, quantity_pow_by_dim.items())]
-    return coeff*Mul.fromiter(other_pow)*Mul.fromiter(new_quantities)
diff --git a/sympy/solvers/tests/test_solvers.py b/sympy/solvers/tests/test_solvers.py
index bc3a799..a277d14 100644
--- a/sympy/solvers/tests/test_solvers.py
+++ b/sympy/solvers/tests/test_solvers.py
@@ -15,7 +15,7 @@
     det_quick, det_perm, det_minor, _simple_dens, check_assumptions, denoms, \
     failing_assumptions
 
-from sympy.physics.units import cm
+from sympy.physics.units import cm, meter
 from sympy.polys.rootoftools import CRootOf
 
 from sympy.utilities.pytest import slow, XFAIL, SKIP, raises, skip, ON_TRAVIS
@@ -1715,7 +1715,7 @@ def test_issue_7110():
 
 
 def test_units():
-    assert solve(1/x - 1/(2*cm)) == [2*cm]
+    assert solve(1/x - 1/(2*cm)) == [2*meter/100]
 
 
 def test_issue_7547():
@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 8, 2018

should quantity_simplify do conversions like joule/coulomb to volt (as it's a derived unit)? And master branch doesn't changes it too.

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 9, 2018

should quantity_simplify do conversions

I'm not sure...what was your intent when writing this function? Was it just to work around the solver issues? If so, I don't think it's needed anymore.

jashan498 added some commits Jul 9, 2018

@jashan498 jashan498 force-pushed the jashan498:linsolve_prefix branch from b9bcd8f to 8853fc4 Jul 13, 2018

@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 13, 2018

@smichr have a look at this commit. Its just a little change over master's quantity_sumplify and also doesn't change its behaviour much like previous commits.

@@ -148,6 +149,8 @@ def quantity_simplify(expr):
coeff = coeff * arg.scale_factor
expr = coeff

if any(isinstance(a, Add) for a in expr.args):

This comment has been minimized.

@smichr

smichr Jul 15, 2018

Member

You have already run quantity_simplify over all arguments at line 138. I don't think these added lines will ever do anything.

This comment has been minimized.

@smichr

smichr Jul 15, 2018

Member

Oh...you are trying to avoid the error you identified in the OP. This will avoid the error but then will fail to simplify something like:

>>> quantity_simplify(foot*inch*(foot + inch))
foot*inch*(foot + inch)
@@ -149,8 +149,16 @@ def quantity_simplify(expr):
coeff = coeff * arg.scale_factor
expr = coeff

if any(isinstance(a, Add) for a in expr.args):
return expr.func(*map(quantity_simplify, expr.args))
if isinstance(expr, Mul) and any(isinstance(a, Add) for a in expr.args):

This comment has been minimized.

@smichr

smichr Jul 15, 2018

Member

The problem is not there, really. How about:

diff --git a/sympy/physics/units/util.py b/sympy/physics/units/util.py
index cc15b65..041a608 100644
--- a/sympy/physics/units/util.py
+++ b/sympy/physics/units/util.py
@@ -133,6 +133,7 @@ def get_total_scale_factor(expr):
 def quantity_simplify(expr):
     if expr.is_Atom:
         return expr
+
     if not expr.is_Mul:
         return expr.func(*map(quantity_simplify, expr.args))

@@ -149,6 +150,7 @@ def quantity_simplify(expr):
     coeff, args = expr.as_coeff_mul(Quantity)
     args_pow = [arg.as_base_exp() for arg in args]
     quantity_pow, other_pow = sift(args_pow, lambda x: isinstance(x[0], Quantity), binary=True)
+    coeff *= Mul.fromiter([Pow(b, e, evaluate=False) for b, e in other_pow])
     quantity_pow_by_dim = sift(quantity_pow, lambda x: x[0].dimension)
     # Just pick the first quantity:
     ref_quantities = [i[0][0] for i in quantity_pow_by_dim.values()]
@@ -157,4 +159,4 @@ def quantity_simplify(expr):
             (quantity*i.scale_factor/quantity.scale_factor)**p for i, p in v)
             if len(v) > 1 else v[0][0]**v[0][1]
         for quantity, (k, v) in zip(ref_quantities, quantity_pow_by_dim.items())]
-    return coeff*Mul.fromiter(other_pow)*Mul.fromiter(new_quantities)
+    return coeff*Mul.fromiter(new_quantities)

This comment has been minimized.

@jashan498

jashan498 Jul 15, 2018

Author Contributor

Oh, you are right. I was seeing it the other way.

@@ -133,12 +133,13 @@ def get_total_scale_factor(expr):


def quantity_simplify(expr):

This comment has been minimized.

@smichr

smichr Jul 16, 2018

Member

This fails for quantity_simplify(2**(foot/inch*kilo/1000)*inch); an error is raised instead of giving 4096*inch

How about the following (after importing ordered at the top of the file along with the others imported from compatibility):

def quantity_simplify(expr):
    """Return an equivalent expression in which prefixes are replaced
    with numerical values and products of related quantities are
    unified in a canonical manner.

    Examples
    ========

    >>> from sympy.physics.units.util import quantity_simplify
    >>> from sympy.physics.units.prefixes import kilo
    >>> from sympy.physics.units import foot, inch
    >>> quantity_simplify(kilo*foot*inch)
    250*foot**2/3
    """

    if expr.is_Atom:
        return expr

    if isinstance(expr, Prefix):
        return expr.scale_factor

    expr = expr.func(*map(quantity_simplify, expr.args))

    if not expr.is_Mul or not expr.has(Quantity):
        return expr

    args_pow = [arg.as_base_exp() for arg in expr.args]
    quantity_pow, other_pow = sift(
        args_pow, lambda x: isinstance(x[0], Quantity), binary=True)
    coeff = Mul.fromiter([
        Pow(b, e, evaluate=False) for b, e in other_pow])
    quantity_pow_by_dim = sift(quantity_pow, lambda x: x[0].dimension)
    new_quantities = []
    for _, bp in quantity_pow_by_dim.items():
        if len(bp) == 1:
            new_quantities.append(bp[0][0]**bp[0][1])
        else:
            # just let reference quantity be the first quantity,
            # picked from an ordered list
            bp = list(ordered(bp))
            ref = bp[0][0]/bp[0][0].scale_factor
            new_quantities.append(Mul.fromiter((
                (ref*b.scale_factor)**p for b, p in bp)))
    return coeff*Mul.fromiter(new_quantities)

The doctest assert quantity_simplify(foot**2*inch + inch**2*foot) == 13*foot**3/144 should also be added to the test suite: it shows why making the list canonically ordered matters. Without ordering, in one term inch would be the reference quantity and in the other, foot.

jashan498 added some commits Jul 16, 2018

@jashan498 jashan498 force-pushed the jashan498:linsolve_prefix branch from 69b225f to 7cb4269 Jul 17, 2018

@smichr

This comment has been minimized.

Copy link
Member

smichr commented Jul 17, 2018

OK, let's launch this!

@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 17, 2018

@smichr this job failed, can you restart it?

@smichr smichr merged commit 1d1eb62 into sympy:master Jul 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jashan498

This comment has been minimized.

Copy link
Contributor Author

jashan498 commented Jul 17, 2018

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.