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

solveset_real need to check symbol in piecewise-condition.Expression with multiple abs ,having Piecewise solution as 0 #10550

Open
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Shekharrajak
Copy link
Member

commented Feb 8, 2016

Fixes #10122 and #10534

To fix the Assertion Error of piecewise test-case in PR #10502
need changes of PR #10547
I merged that PR branch into this.

@Shekharrajak Shekharrajak changed the title 10534 merge 10122 solveset_real need to check symbol in condition ,Expression with multiple abs ,having Piecewise solution as 0 Feb 8, 2016

@Shekharrajak Shekharrajak changed the title solveset_real need to check symbol in condition ,Expression with multiple abs ,having Piecewise solution as 0 solveset_real need to check symbol in piecewise-condition ,Expression with multiple abs ,having Piecewise solution as 0 Feb 8, 2016

@Shekharrajak Shekharrajak changed the title solveset_real need to check symbol in piecewise-condition ,Expression with multiple abs ,having Piecewise solution as 0 solveset_real need to check symbol in piecewise-condition.Expression with multiple abs ,having Piecewise solution as 0 Feb 8, 2016

for (expr, in_set) in expr_set_pairs:
solns = solveset_real(expr, symbol).intersect(in_set)
result = result + solns
expr_cond = {}

This comment has been minimized.

Copy link
@smichr

smichr Feb 8, 2016

Member

just write expr_cond = dict(f.args)?

x, y = symbols('x y', real=True)
assert solve(abs(x + 3) - 2*abs(x - 3)) == [1, 9]
assert solve([abs(x) - 2, arg(x) - pi], x) == [(-2,), (2,)]
assert set(solve(abs(x - 7) - 8)) == set([-S(1), S(15)])

# issue 8692
assert solve(Eq(Abs(x + 1) + Abs(x**2 - 7), 9), x) == [

This comment has been minimized.

Copy link
@smichr

smichr Feb 8, 2016

Member

You have removed a test for solve and replaced it with a solution for solveset. You should move the solveset solution to the test_solveset.py file. The solve test should not be deleted. If it no longer works, then this is a regression that needs to be fixed. Do you know why it didn't give an error before but gives an error now?

This comment has been minimized.

Copy link
@smichr

smichr Feb 8, 2016

Member

In your branch I don't get a failure:

>>> from sympy import *
>>> var('x')
x
>>> var('x', real=True)
x
>>> solve(Eq(Abs(x + 1) + Abs(x**2 - 7), 9), x)
[-1/2 + sqrt(61)/2, -sqrt(69)/2 + 1/2]

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Feb 8, 2016

Author Member
x, y, z, t = symbols('x y z t', real = True)

works.But

x, y, z, t = symbols('x y z t')

doesn't work.

This comment has been minimized.

Copy link
@smichr

smichr Feb 8, 2016

Member

This is expected. That is why in the test function the variables are defined as being real., 5 lines above "# issue 8692 ".

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Feb 8, 2016

Author Member

thanks for pointing it out.It starts working when I merged both PRs.

@smichr

This comment has been minimized.

Copy link

commented on sympy/solvers/tests/test_solvers.py in 3d82e70 Feb 9, 2016

solve and solveset have their own test suites; the solveset test should be moved with to test_solveset.py

This comment has been minimized.

Copy link
Owner Author

replied Feb 9, 2016

I forgot to remove that.Now it is done.I don't think that test-case is needed in solveset.

@@ -304,7 +304,40 @@ def test_piecewise_solve():
(-x + 2, x - 2 <= 0), (x - 2, x - 2 > 0))
assert solve(g, x) == [5]

# See issue 4352 (enhance the solver to handle inequalities).

def test_piecewise_solveset():

This comment has been minimized.

Copy link
@smichr

smichr Feb 11, 2016

Member

This should be in the solveset test suite; you aren't testing Piecewise, you are testing how solveset handles it.

This comment has been minimized.

Copy link
@Shekharrajak

Shekharrajak Feb 11, 2016

Author Member

I found test case for solve just above so copied the same cases for solveset to check it is working correctly or not.
I have shifted the both testcases.

@smichr

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

Also, see comment on line 490.

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Feb 11, 2016

I changed the expr_cond = {} to expr_cond = dict(f.args).Thanks for pointing this. @smichr

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2016

Why is solveset_real totally changed in master branch?

def solveset_real(f, symbol):
    return solveset(f, symbol, S.Reals)
@smichr

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

Why is solveset_real totally changed in master branch?

Because it went through a major overhaul. It, and solveset_complex are now just thin wrappers for calling solveset with the real or complex domain. It was this overhaul that I mentioned in another of your PRs.

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2016

I spent hours for these PRs. I did similar thing with _solve_real_trig in PR 10552.
and also that PR tried to handle trigonometric and hyperbolic function same as solveset_real in solveset_complex (which is now not needed).
Now _solveset is handling both domain so I need to place my changes there.
Should I go ahead and fix this conflict in both PR ,will these changes be accepted?

@smichr

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

Should I go ahead and fix this conflict in both PR ,will these changes be accepted?

For this PR, yes. It looks like the other one is already closed.

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:10534_merge_10122 branch 2 times, most recently from 2476407 to 79bd2c2 Apr 9, 2016

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2016

@smichr , please review once.

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.