`_solve` and `_solve_system`: return list of dictionaries #23877

smichr · 2022-08-04T12:50:42Z

References to other Issues or PRs

Brief description of what is fixed or changed

This is mostly an internal clean-up of solve to work with lists of dictionaries from _solve and _solve_system while maintaining legacy output.

Other comments

A solution is only made canonical by solve; _solve, _solve_system and _tsolve no longer are guaranteed to return a canonical list. This seems appropriate for helper functions.

The bare dictionary return value will only occur when a single equation is being solved for undetermined coefficients

>>> solve(a*x+b-3+2*x, a, b)
{a: -2, b: 3}

or when n-linear-equations in n-unknowns are being solved and manual is not True

>>> solve([x+y-1,x-y-2],x,y,z)  # n +1 unknowns
{x: 3/2, y: -1/2}
>>> solve([x+y-1,x-y-2],x,y)
{x: 3/2, y: -1/2}
>>> solve([x+y-1,x-y-2],x,y,manual=1)
[(3/2, -1/2)]

Release Notes

solvers
- solve now only returns a bare dictionary when 1) solving a single expression (not in a list) for undetermined coefficients or 2) for a list of n linear equations in n unknowns when manual!=True
- when using solve to solve for undetermined coefficients, the solution is no longer simplified (because it is detected as having been solved as a polynomial system; and such systems are not simplified)

sympy-bot · 2022-08-04T12:50:52Z

✅

Hi, I am the SymPy bot (v167). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

solvers
- solve now only returns a bare dictionary when 1) solving a single expression (not in a list) for undetermined coefficients or 2) for a list of n linear equations in n unknowns when manual!=True (#23877 by @smichr)
- when using solve to solve for undetermined coefficients, the solution is no longer simplified (because it is detected as having been solved as a polynomial system; and such systems are not simplified) (#23877 by @smichr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.12.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

This is mostly an internal clean-up of `solve` to work with lists of dictionaries from `_solve` and `_solve_system` while maintaining legacy output.

#### Other comments

A solution is only made canonical by solve; `_solve`, `_solve_system` and `_tsolve` no longer are guaranteed to return a canonical list. This seems appropriate for helper functions.

The bare dictionary return value will only occur when a single equation is being solved for undetermined coefficients
```python
>>> solve(a*x+b-3+2*x, a, b)
{a: -2, b: 3}
```
or when n-linear-equations in n-unknowns are being solved and manual is not True
```python
>>> solve([x+y-1,x-y-2],x,y,z)  # n +1 unknowns
{x: 3/2, y: -1/2}
>>> solve([x+y-1,x-y-2],x,y)
{x: 3/2, y: -1/2}
>>> solve([x+y-1,x-y-2],x,y,manual=1)
[(3/2, -1/2)]
```

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* solvers
  * `solve` now only returns a bare dictionary when 1) solving a single expression (not in a list) for undetermined coefficients or 2) for a list of n linear equations in n unknowns when manual!=True
  * when using `solve` to solve for undetermined coefficients, the solution is no longer simplified (because it is detected as having been solved as a polynomial system; and such systems are not simplified)
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

sympy/solvers/solvers.py

oscarbenjamin · 2022-08-04T17:27:19Z

I haven't reviewed this in detail but from a glance it looks like a nice improvement.

sympy/solvers/solvers.py

sympy/solvers/tests/test_solvers.py

sympy/solvers/solvers.py

sympy/solvers/tests/test_solvers.py

smichr · 2022-08-09T15:07:52Z

I think it's better not to simplify.

OK, then I have nothing more planned for this PR.

sympy/solvers/solvers.py

oscarbenjamin · 2022-08-10T14:29:11Z

sympy/solvers/solvers.py

-    >>> tsolve(3**(2*x + 5) - 4, x)
-    [-5/2 + log(2)/log(3), (-5*log(3)/2 + log(2) + I*pi)/log(3)]
+    >>> set(tsolve(3**(2*x + 5) - 4, x))
+    {(-5*log(3)/2 + log(2) + I*pi)/log(3), -5/2 + log(2)/log(3)}


Is this doctest reliable?

I would have expected the set to print in different orders on different runs.

The test seems to work okay when I run it several times. Maybe that's because the SymPy printer makes the order deterministic...

It is written to be reliable since {1,2}=={2,1}

Yes, but with standard Python repr printing the order when printed is non-deterministic and doctest matches the text output directly (rather than doing any set comparison).

oscarbenjamin · 2022-08-10T14:38:03Z

I think this looks good. More cleanup is still needed e.g. to break solve into functions and reorder the steps. This restructure makes things a lot cleaner as a foundation for further work though.

oscarbenjamin · 2022-08-10T14:38:19Z

Let's get this in.

smichr · 2022-08-10T14:39:12Z

Thanks for the help. We're making progress.

oscarbenjamin · 2022-08-10T21:22:40Z

After merging this the slow2 test failed on the master branch:
https://github.com/sympy/sympy/runs/7769802231?check_suite_focus=true

It seems that a slow test in test_solvers.py failed but also the whole job timed out.

I might have seen that failure recently in which case maybe it isn't related to this PR but since it is test_solvers.py it could also be related.

oscarbenjamin · 2022-08-11T09:49:06Z

The failure in #23832 is different:

__________ sympy/solvers/tests/test_solvers.py:test_lambert_bivariate __________
Traceback (most recent call last):
  File "/home/runner/work/sympy/sympy/sympy/testing/runtests.py", line 1328, in _timeout
    function()
  File "/home/runner/work/sympy/sympy/sympy/testing/pytest.py", line 188, in func_wrapper
    func()
  File "/home/runner/work/sympy/sympy/sympy/solvers/tests/test_solvers.py", line 1907, in test_lambert_bivariate
    assert set(solve(3**cos(x) - cos(x)**3)) == {
AssertionError

 tests finished: 61 passed, 1 failed, 6 skipped, 14 expected to fail, 
in 2801.67 seconds 
DO *NOT* COMMIT!
Error: Process completed with exit code 1.

oscarbenjamin · 2022-08-11T09:58:00Z

I can confirm that the following fails non-deterministically:

$ pytest sympy/solvers/tests/test_solvers.py::test_lambert_bivariate

I just tried repeatedly and it passed 3 times before failing on the 4th run:

$ pytest sympy/solvers/tests/test_solvers.py::test_lambert_bivariate
======================================== test session starts ========================================
platform linux -- Python 3.8.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
benchmark: 3.4.1 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
architecture: 64-bit
cache:        yes
ground types: gmpy 2.1.2

rootdir: /home/oscar/current/sympy/sympy.git, configfile: pytest.ini
plugins: benchmark-3.4.1, forked-1.3.0, cov-3.0.0, instafail-0.4.2, xdist-2.4.0, anyio-3.5.0, doctestplus-0.12.0
collected 1 item                                                                                    

sympy/solvers/tests/test_solvers.py F                                                         [100%]

============================================= FAILURES ==============================================
______________________________________ test_lambert_bivariate _______________________________________

    @slow
    def test_lambert_bivariate():
        # tests passing current implementation
        assert solve((x**2 + x)*exp(x**2 + x) - 1) == [
            Rational(-1, 2) + sqrt(1 + 4*LambertW(1))/2,
            Rational(-1, 2) - sqrt(1 + 4*LambertW(1))/2]
        assert solve((x**2 + x)*exp((x**2 + x)*2) - 1) == [
            Rational(-1, 2) + sqrt(1 + 2*LambertW(2))/2,
            Rational(-1, 2) - sqrt(1 + 2*LambertW(2))/2]
        assert solve(a/x + exp(x/2), x) == [2*LambertW(-a/2)]
        assert solve((a/x + exp(x/2)).diff(x), x) == \
                [4*LambertW(-sqrt(2)*sqrt(a)/4), 4*LambertW(sqrt(2)*sqrt(a)/4)]
        assert solve((1/x + exp(x/2)).diff(x), x) == \
            [4*LambertW(-sqrt(2)/4),
            4*LambertW(sqrt(2)/4),  # nsimplifies as 2*2**(141/299)*3**(206/299)*5**(205/299)*7**(37/299)/21
            4*LambertW(-sqrt(2)/4, -1)]
        assert solve(x*log(x) + 3*x + 1, x) == \
                [exp(-3 + LambertW(-exp(3)))]
        assert solve(-x**2 + 2**x, x) == [2, 4, -2*LambertW(log(2)/2)/log(2)]
        assert solve(x**2 - 2**x, x) == [2, 4, -2*LambertW(log(2)/2)/log(2)]
        ans = solve(3*x + 5 + 2**(-5*x + 3), x)
        assert len(ans) == 1 and ans[0].expand() == \
            Rational(-5, 3) + LambertW(-10240*root(2, 3)*log(2)/3)/(5*log(2))
        assert solve(5*x - 1 + 3*exp(2 - 7*x), x) == \
            [Rational(1, 5) + LambertW(-21*exp(Rational(3, 5))/5)/7]
        assert solve((log(x) + x).subs(x, x**2 + 1)) == [
            -I*sqrt(-LambertW(1) + 1), sqrt(-1 + LambertW(1))]
        # check collection
        ax = a**(3*x + 5)
        ans = solve(3*log(ax) + b*log(ax) + ax, x)
        x0 = 1/log(a)
        x1 = sqrt(3)*I
        x2 = b + 3
        x3 = x2*LambertW(1/x2)/a**5
        x4 = x3**Rational(1, 3)/2
        assert ans == [
            x0*log(x4*(-x1 - 1)),
            x0*log(x4*(x1 - 1)),
            x0*log(x3)/3]
        x1 = LambertW(Rational(1, 3))
        x2 = a**(-5)
        x3 = -3**Rational(1, 3)
        x4 = 3**Rational(5, 6)*I
        x5 = x1**Rational(1, 3)*x2**Rational(1, 3)/2
        ans = solve(3*log(ax) + ax, x)
        assert ans == [
            x0*log(3*x1*x2)/3,
            x0*log(x5*(x3 - x4)),
            x0*log(x5*(x3 + x4))]
        # coverage
        p = symbols('p', positive=True)
        eq = 4*2**(2*p + 3) - 2*p - 3
        assert _solve_lambert(eq, p, _filtered_gens(Poly(eq), p)) == [
            Rational(-3, 2) - LambertW(-4*log(2))/(2*log(2))]
>       assert set(solve(3**cos(x) - cos(x)**3)) == {
            acos(3), acos(-3*LambertW(-log(3)/3)/log(3))}
E       assert {2*pi - acos(...*pi - acos(3)} == {acos(3), aco...3)/3)/log(3))}
E         Extra items in the left set:
E         2*pi - acos(-3*LambertW(-log(3)/3)/log(3))
E         2*pi - acos(3)
E         Extra items in the right set:
E         acos(3)
E         acos(-3*LambertW(-log(3)/3)/log(3))
E         Use -v to get the full diff

sympy/solvers/tests/test_solvers.py:1907: AssertionError
                                          DO *NOT* COMMIT!                                          
====================================== short test summary info ======================================
FAILED sympy/solvers/tests/test_solvers.py::test_lambert_bivariate - assert {2*pi - acos(...*pi - ...
======================================== 1 failed in 16.30s =========================================

oscarbenjamin · 2022-08-11T10:06:56Z

The following script returns nondeterministic output:

from sympy import *
x = symbols('x')
print(solve(3**cos(x) - cos(x)**3))

Running 10 times:

$ for i in `seq 1 10`; do python t.py; done
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[2*pi - acos(3), 2*pi - acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[2*pi - acos(3), 2*pi - acos(-3*LambertW(-log(3)/3)/log(3))]
[2*pi - acos(3), 2*pi - acos(-3*LambertW(-log(3)/3)/log(3))]

Checking out a commit shortly before this PR it seems to be deterministic:

$ git checkout 6fd8df361985e2c3bfdfb4767c32f0e5e5ccd
$ for i in `seq 1 10`; do python t.py; done
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]
[acos(3), acos(-3*LambertW(-log(3)/3)/log(3))]

oscarbenjamin · 2022-08-11T10:09:00Z

Setting the hash seed does not make the output deterministic.

oscarbenjamin · 2022-08-11T10:23:39Z

sympy/solvers/solvers.py

-                            sols.append(cv_inv.subs(t, sol))
-                        result = list(ordered(sols))
+                        cv_inv = _vsolve(t - f1, symbol, **flags)[0]
+                        result = [{symbol: cv_inv.subs(sol)} for sol in cv_sols]


Here _vsolve returns a list [acos(_t), -acos(_t) + 2*pi] but the ordering of this list is nondeterministic. The line above arbitrarily selects the first element of this list.

oscarbenjamin · 2022-08-11T10:42:39Z

This is enough to make the output deterministic:

diff --git a/sympy/solvers/solvers.py b/sympy/solvers/solvers.py
index 6a2f116..dc8684f 100644
--- a/sympy/solvers/solvers.py
+++ b/sympy/solvers/solvers.py
@@ -1559,7 +1559,7 @@ def _as_base_q(x):
                     # if no Functions left, we can proceed with usual solve
                     if not ftry.has(symbol):
                         cv_sols = _solve(ftry, t, **flags)
-                        cv_inv = _vsolve(t - f1, symbol, **flags)[0]
+                        cv_inv, _ = ordered(_vsolve(t - f1, symbol, **flags))
                         result = [{symbol: cv_inv.subs(sol)} for sol in cv_sols]
 
                 if result is False:

I'm still unsure why the function arbitrarily picks only one of the two solutions though. Both answers give valid solutions:

In [1]: eq = 3**cos(x) - cos(x)**3

In [2]: eq.subs(x, acos(3))
Out[2]: 0

In [3]: eq.subs(x, 2*pi - acos(3))
Out[3]: 0

There are actually infinitely many solutions so I'm not really sure what solve should return. The solutions in the complex plane are given as the intersections of the red and blue lines in this plot:

oscarbenjamin · 2022-08-12T22:11:13Z

@smichr what should be the fix for the test that fails as a result of this PR? It's showing up as a failure in CI about 50% of the time.

smichr · 2022-08-18T12:48:48Z

what should be the fix for the test

see #23918

oscarbenjamin · 2022-08-18T13:24:06Z

what should be the fix for the test

see #23918

I don't understand what you mean by that. I've opened a PR #23944 to make this at least deterministic.

smichr commented Aug 4, 2022

View reviewed changes

sympy/solvers/solvers.py Outdated Show resolved Hide resolved

smichr commented Aug 4, 2022

View reviewed changes

sympy/solvers/solvers.py Show resolved Hide resolved

smichr commented Aug 4, 2022

View reviewed changes

sympy/solvers/solvers.py Outdated Show resolved Hide resolved

oscarbenjamin added solvers solvers.solve labels Aug 4, 2022

smichr commented Aug 4, 2022

View reviewed changes

sympy/solvers/solvers.py Show resolved Hide resolved

smichr commented Aug 4, 2022

View reviewed changes

sympy/solvers/solvers.py Show resolved Hide resolved

smichr changed the title ~~_solve and _solve_system return lists of dictionaries~~ _solve and _solve_system: return list of dictionaries Aug 4, 2022

smichr force-pushed the s5 branch 2 times, most recently from dee0d38 to d39448b Compare August 6, 2022 01:22