2957 - handle some equations that lead to oo recursion in master. #931

Merged
merged 9 commits into from Jan 12, 2012

Projects

None yet

2 participants

@smichr
Member
smichr commented Jan 4, 2012

No description provided.

@goodok
Contributor
goodok commented Jan 10, 2012

All tests passes well (See http://reviews.sympy.org/pullrequest/931 )
But I will see tomorrow in more detailed .

@goodok goodok commented on an outdated diff Jan 11, 2012
sympy/solvers/tests/test_solvers.py
assert solve(sqrt(17*x-sqrt(x**2-5))-7) == [3]
-@XFAIL
-def test_unrad2():
- assert solve((x**3-3*x**2)**Rational(1,3)+1-x) == [S(1)/3] # b/c (-8/27)**(1/3) -> 2*(-1)**(1/3)/3 instead of -2/3
+ assert solve(sqrt(x) - sqrt(x - 1) + sqrt(sqrt(x))) is not None
@goodok
goodok Jan 11, 2012 Contributor

I don't understand why is not None is here. Why don't to use simply == []? (as it is used above in the other tests too)

@goodok
Contributor
goodok commented Jan 11, 2012

I have no other remarks for this pull request.

For information, I have possible remarks only about the other parts of code of the solvers: one of them are about comments of code and one question about substitution method (why attempt is to substitute only first function).

But I will formulate and add the comments/remarks to this code later.

@goodok
Contributor
goodok commented Jan 12, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvPkJDA

Interpreter: /usr/local/bin/python3 (3.2.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 1fc03c5
branch hash: 064fa5b

Automatic review by SymPy Bot.

@goodok
Contributor
goodok commented Jan 12, 2012

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYpYEKDA

Interpreter: /usr/local/bin/python (2.7.2-final-0)
Architecture: Linux (64-bit)
Cache: yes
Test command: setup.py test
master hash: 1fc03c5
branch hash: 064fa5b

Automatic review by SymPy Bot.

@goodok
Contributor
goodok commented Jan 12, 2012

SymPy Bot Summary: There were test failures.

@smichr: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYuKAKDA

Interpreter: F:\Python32\python.exe (3.2.2-final-0)
Architecture: Windows (32-bit)
Cache: yes
Test command: setup.py test
master hash: 1fc03c5
branch hash: 064fa5b

Automatic review by SymPy Bot.

@smichr
Member
smichr commented Jan 12, 2012

Well this is curious...how can we get different result from py3.2 on
Win-32? I'll double check that I have py3.2.2.

@goodok
Contributor
goodok commented Jan 12, 2012

I think it is the random error, which is described in:
http://code.google.com/p/sympy/issues/detail?id=2956

I started sympy-bot test once-more.

@goodok
Contributor
goodok commented Jan 12, 2012

SymPy Bot Summary: There were merge conflicts; could not test the branch.

@smichr: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYzvEJDA

Interpreter: F:\Python32\python.exe (3.2.2-final-0)
Architecture: Windows (32-bit)
Cache: yes
Test command: setup.py test
master hash: 93872a8
branch hash: 064fa5b

Automatic review by SymPy Bot.

@goodok
Contributor
goodok commented Jan 12, 2012

Btw, please rebase it. sympy-bot can't automatically merge this pull request to test now.

@goodok
Contributor
goodok commented Jan 12, 2012

Yes, it is related with random seeds,

btw, I've got this test failure for this seed and for linux/64-bit too.
($ ./bin/test test_hyperexpand.py --seed=34335697)
And got no errors for other seeds.

I cloned this branch and will report to the issue 2956 page about it,

@smichr
Member
smichr commented Jan 12, 2012

OK, so ignoring that hyperexpand failure which is not related to this
work, does the rest look ok to you?

smichr added some commits Jan 4, 2012
@smichr smichr move multiple func handling out of _tsolve and watch for oo recursion
    The handling of multiple generators belongs up in _solve rather than
    _tsolve.

    In _tsolve, a power shouldn't be sent back to solve unless it is
    different than what was input into _tsolve or else infinite
    recursion will occur.
dbcb160
@smichr smichr watch out for log not changing tsolve expression; try rewrite 894a2cf
@smichr smichr watch for inverted generators leading to single base 7f15c5a
@goodok
Contributor
goodok commented Jan 12, 2012

Yes, it is ok. (only above remark about 'is not None' - it might hide cases when the result of selover are not empty )

But don't forget rebase it.

smichr added some commits Jan 4, 2012
@smichr smichr make pep8 compliant 8166ac1
@smichr smichr remove dead line from unrad 83b7e43
@smichr smichr unrad improvements ac82460
@smichr smichr for single term, mind negative sign since root may not be sqrt
    unrad(x**(1/3)-(x-1)) should be x - (x - 1)**3 whereas
    unrad(x**(1/2)-(x-1)) is the same whether one has x - (x - 1)**2 or

    In addition, real_root must be used in checksol in order for
    a solution of 1/3 to be recognized as the solution to
    solve((x**3-3*x**2)**Rational(1,3)+1-x) since a straight substitution
    of 1/3 into that expression gives

    >>> ((x**3-3*x**2)**Rational(1,3)+1-x).subs(x, S(1)/3)
    2/3 + 2*(-1)**(1/3)/3

    Application of real_root demonstrates that it is a solution:
    >>> real_root(_)
    0
eec466e
@smichr smichr remove evalf code for case when foo.is_nonzero is not None
    Although issue 2574 has not changed, the code that was removed was
    using numerical evaluation which is something that checksol would
    already do:

    >>> checksol(-x + exp(exp(LambertW(log(x)))*LambertW(log(x))), x, 2)
    True

    That True result was obtained numerically. If numerical=False, the
    expression will evaluate as nonzero (which is wrong):

    >>> (-x + exp(exp(LambertW(log(x)))*LambertW(log(x)))).subs(x, 2)
    -2 + exp(exp(LambertW(log(2)))*LambertW(log(2)))
    >>> e=_;e.is_nonzero
    True
    >>> e.n()
    -.0e-123

    So if we encounter such an expression we return None instead of the
    False that would be returned since the value of 2 appears (to the
    assumption system) to produce a nonzero result:

    >>> checksol(-x + exp(exp(LambertW(log(x)))*LambertW(log(x))),
    ... x, 2, numerical=False) is None
    True
0a6ae15
@smichr smichr better document check=False usage in solve 81ce1b2
@smichr
Member
smichr commented Jan 12, 2012

OK, I made that change to the test after confirming that none of the unrad-ed solutions were solutions of the original equation and graphing the equation to see that there were no roots. Thanks for looking this over. It's in.

@smichr smichr merged commit 2cbc4a7 into sympy:master Jan 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment