issue 4046: better handling of Abs by solve #2927

Merged
merged 2 commits into from Mar 6, 2014

Conversation

Projects
None yet
2 participants
@smichr
Member

smichr commented Feb 18, 2014

No description provided.

sympy/solvers/solvers.py
+ fi = fi.subs(a, newabs(a.args[0]))
+ hit = True
+ else:
+ fi = fi.replace(a, sqrt(a.args[0]**2))

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 19, 2014

Member

x not real is exactly when sqrt(x**2) is not equal to abs(x).

@asmeurer

asmeurer Feb 19, 2014

Member

x not real is exactly when sqrt(x**2) is not equal to abs(x).

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 19, 2014

Member

If the arg of abs is real you don't want to wrap it in sqrt(arg2) or it will become abs(arg) again -- that was the issue that this solves (and the test confirms that this is correct). The objective is to remove the abs from the expression. The else part handles the case when the is_real state is not known as in vanilla x in abs(x) - 3; that gets converted (and stays) as sqrt(x2) - 3 and returns [-3, 3] for x. If is_real is False then the checksol will eliminate the spurious solutions ([] is returned in the previous example if x = Symbol('x', real=False)). When is_real is known we can't wrap the argument in sqrt(); your suggestion which I implemented here is to wrap it in Piecewise -- and that has worked well.

@smichr

smichr Feb 19, 2014

Member

If the arg of abs is real you don't want to wrap it in sqrt(arg2) or it will become abs(arg) again -- that was the issue that this solves (and the test confirms that this is correct). The objective is to remove the abs from the expression. The else part handles the case when the is_real state is not known as in vanilla x in abs(x) - 3; that gets converted (and stays) as sqrt(x2) - 3 and returns [-3, 3] for x. If is_real is False then the checksol will eliminate the spurious solutions ([] is returned in the previous example if x = Symbol('x', real=False)). When is_real is known we can't wrap the argument in sqrt(); your suggestion which I implemented here is to wrap it in Piecewise -- and that has worked well.

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 19, 2014

Member

In that case we don't get enough solutions (there are infinitely many solutions to abs(x) = 3), which is not terrible, but I'm not convinced that it couldn't also lead to incorrect solutions.

@asmeurer

asmeurer Feb 19, 2014

Member

In that case we don't get enough solutions (there are infinitely many solutions to abs(x) = 3), which is not terrible, but I'm not convinced that it couldn't also lead to incorrect solutions.

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 19, 2014

Member

What does it give for abs(x) - y?

@asmeurer

asmeurer Feb 19, 2014

Member

What does it give for abs(x) - y?

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 19, 2014

Member
>>> solve(abs(x)-y,x)
[-y, y]
@smichr

smichr Feb 19, 2014

Member
>>> solve(abs(x)-y,x)
[-y, y]

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 19, 2014

Member

@asmeurer , if we aren't going to handle the non-real argument (and I don't think we are set up to do so at present) then perhaps we should just raise an error. If we don't do so then we get this sort of (current) nonsense:

>>> solve(abs(x-2)-(3+I))
[2 - sqrt(8 + 6*I), 2 + sqrt(8 + 6*I)]
>>> [abs(w-2).n(2) for w in _]
[3.2, 3.2]
@smichr

smichr Feb 19, 2014

Member

@asmeurer , if we aren't going to handle the non-real argument (and I don't think we are set up to do so at present) then perhaps we should just raise an error. If we don't do so then we get this sort of (current) nonsense:

>>> solve(abs(x-2)-(3+I))
[2 - sqrt(8 + 6*I), 2 + sqrt(8 + 6*I)]
>>> [abs(w-2).n(2) for w in _]
[3.2, 3.2]

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 19, 2014

Member

Which is wrong if y is not positive.

@asmeurer

asmeurer Feb 19, 2014

Member

Which is wrong if y is not positive.

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 19, 2014

Member

Yes, this is the sort of thing I expected. abs(x) can only be positive. If you get any result from abs(x) - y where y is not positive, it will be incorrect nonsense.

I'd be a lot happier here if this else branch were removed.

@asmeurer

asmeurer Feb 19, 2014

Member

Yes, this is the sort of thing I expected. abs(x) can only be positive. If you get any result from abs(x) - y where y is not positive, it will be incorrect nonsense.

I'd be a lot happier here if this else branch were removed.

sympy/solvers/solvers.py
+ is real, abs(x) permits 2 solutions but when
+ x is complex then there are infinitely many
+ solutions, e.g. abs(x) = 3 for
+ |re(x)| <= 3 and re(x)**2 + im(x)**2 = 9.

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 20, 2014

Member

The first condition is not necessary. Complex abs is sqrt(re(x)**2 + im(x)**2).

@asmeurer

asmeurer Feb 20, 2014

Member

The first condition is not necessary. Complex abs is sqrt(re(x)**2 + im(x)**2).

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 20, 2014

Member

I took my cues from wolframalpha; sqrt(re(x)**2 + im(x)**2) = 3 is 1 equation in two unknowns; the first condition is the second equation. (Or are you talking about a different condition?)

@smichr

smichr Feb 20, 2014

Member

I took my cues from wolframalpha; sqrt(re(x)**2 + im(x)**2) = 3 is 1 equation in two unknowns; the first condition is the second equation. (Or are you talking about a different condition?)

@smichr smichr closed this Feb 20, 2014

@smichr smichr reopened this Feb 20, 2014

@@ -1103,12 +1103,6 @@ def test_check_assumptions():
assert solve(x**2 - 1) == [1]
-def test_solve_abs():

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 20, 2014

Member

these tests were moved below

@smichr

smichr Feb 20, 2014

Member

these tests were moved below

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 21, 2014

Member

the error message was modified slightly

Member

smichr commented Feb 21, 2014

the error message was modified slightly

sympy/solvers/solvers.py
+ return any([_has_piecewise(a) for a in e.args])
+ if _has_piecewise(f[i]):
+ f[i] = piecewise_fold(f[i])
+ print(f[i])

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 22, 2014

Member

This should not be here.

@asmeurer

asmeurer Feb 22, 2014

Member

This should not be here.

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 22, 2014

Member

This keeps sneaking back in; synching problems on my end.

@smichr

smichr Feb 22, 2014

Member

This keeps sneaking back in; synching problems on my end.

sympy/solvers/solvers.py
+ x is complex then there are infinitely many
+ solutions, e.g. abs(x) = 3 accepts any x that
+ satisfies re(x)**2 + im(x)**2 = 9, e.g.
+ x = sqrt(6) + I*sqrt(3), x = 3, etc....

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 22, 2014

Member

This seems more like thing that should be a comment in the code for whoever looks at implementing this. But I'll take your best judgement on it.

@asmeurer

asmeurer Feb 22, 2014

Member

This seems more like thing that should be a comment in the code for whoever looks at implementing this. But I'll take your best judgement on it.

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 22, 2014

Member

I'm just anticipating people saying "why doesn't it just assume that the answer is real?" Actually...that should work. I'll look more carefully at why wrong answers are not being filtered out. e.g. solve(abs(x)-I) should return [].

@smichr

smichr Feb 22, 2014

Member

I'm just anticipating people saying "why doesn't it just assume that the answer is real?" Actually...that should work. I'll look more carefully at why wrong answers are not being filtered out. e.g. solve(abs(x)-I) should return [].

sympy/solvers/solvers.py
- else:
- raise NotImplementedError(filldedent('''
- Removal of absolute values from %s failed.''' % fi))
+ w = Dummy()

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 22, 2014

Member

w = Dummy('w'). I prefer to name all dummys in code so that when the leak out or when you are debugging it is easier to see where they came from.

@asmeurer

asmeurer Feb 22, 2014

Member

w = Dummy('w'). I prefer to name all dummys in code so that when the leak out or when you are debugging it is easier to see where they came from.

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 22, 2014

Member

Lambda will give it a name, but I will do so here, too.

@smichr

smichr Feb 22, 2014

Member

Lambda will give it a name, but I will do so here, too.

@smichr smichr closed this Feb 22, 2014

@smichr smichr deleted the smichr:abs branch Feb 22, 2014

@smichr smichr reopened this Feb 22, 2014

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 23, 2014

Member

I think this is close to what we want

>>> ok = var('x y')
>>> solve(abs(x)-3)
[{re(x): -sqrt(-im(x)**2 + 9), x: -sqrt(-im(x)**2 + 9) + I*im(x)}, {re(x): sqrt(
-im(x)**2 + 9), x: sqrt(-im(x)**2 + 9) + I*im(x)}]

but I'm stymied by the alternate output when the symbol is specified

>>> solve(abs(x)-3,x)
[{re(x): -sqrt(-(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x
))**2 + 9), x: I*(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(
x)) - sqrt(-(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x))**
2 + 9)}, {re(x): sqrt(-(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9))
+ im(x))**2 + 9), x: I*(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9))
+ im(x)) + sqrt(-(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x
))**2 + 9)}]
Member

smichr commented Feb 23, 2014

I think this is close to what we want

>>> ok = var('x y')
>>> solve(abs(x)-3)
[{re(x): -sqrt(-im(x)**2 + 9), x: -sqrt(-im(x)**2 + 9) + I*im(x)}, {re(x): sqrt(
-im(x)**2 + 9), x: sqrt(-im(x)**2 + 9) + I*im(x)}]

but I'm stymied by the alternate output when the symbol is specified

>>> solve(abs(x)-3,x)
[{re(x): -sqrt(-(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x
))**2 + 9), x: I*(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(
x)) - sqrt(-(-sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x))**
2 + 9)}, {re(x): sqrt(-(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9))
+ im(x))**2 + 9), x: I*(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9))
+ im(x)) + sqrt(-(sin(atan2(0, -im(x)**2 + 9)/2)*sqrt(Abs(-im(x)**2 + 9)) + im(x
))**2 + 9)}]
@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 23, 2014

Member
Member

smichr commented Feb 23, 2014

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 23, 2014

Member

I don't know if that's a particularly helpful solution. I don't even think there is a "useful" solution to abs(x) - 3 for complex x. Probably the most useful one would be exp(I*pi*theta) where theta is a real parameter. That's a good use-case for parameterized solutions (@hargup).

On the other hand, the limited solution set [-3, 3] is useful.

Member

asmeurer commented Feb 23, 2014

I don't know if that's a particularly helpful solution. I don't even think there is a "useful" solution to abs(x) - 3 for complex x. Probably the most useful one would be exp(I*pi*theta) where theta is a real parameter. That's a good use-case for parameterized solutions (@hargup).

On the other hand, the limited solution set [-3, 3] is useful.

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 24, 2014

Member

What I like about the result is that it reminds you that the x provided was unknown as being real or imaginary so you end up with the re and im parts. If you substitute in im(x) -> 0 then the x = +/-3 appears. On the other hand, if you make x real or imaginary you get the simpler results:

>>> var('x',real=True)
x
>>> solve(abs(x)-3)
[-3, 3]
>>> var('x', imaginary=True)
x
>>> solve(abs(x)-3)
[-3*I, 3*I]

We decided to supply the auxiliary equation (x = re(x) + I*im(x)) if the user uses re(x) or im(x); getting a result in terms of re(x) and im(x) is somewhat consistent with that. Rather than raise an error, we just give a solution in terms of other variables (the re and im parts).

Member

smichr commented Feb 24, 2014

What I like about the result is that it reminds you that the x provided was unknown as being real or imaginary so you end up with the re and im parts. If you substitute in im(x) -> 0 then the x = +/-3 appears. On the other hand, if you make x real or imaginary you get the simpler results:

>>> var('x',real=True)
x
>>> solve(abs(x)-3)
[-3, 3]
>>> var('x', imaginary=True)
x
>>> solve(abs(x)-3)
[-3*I, 3*I]

We decided to supply the auxiliary equation (x = re(x) + I*im(x)) if the user uses re(x) or im(x); getting a result in terms of re(x) and im(x) is somewhat consistent with that. Rather than raise an error, we just give a solution in terms of other variables (the re and im parts).

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 24, 2014

Member

OK, I got the weird result fixed -- it had to do with converting the solution back to a list even though that was not needed (since a dict must be returned).

Member

smichr commented Feb 24, 2014

OK, I got the weird result fixed -- it had to do with converting the solution back to a list even though that was not needed (since a dict must be returned).

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 24, 2014

Member

But it's an implicit solution. I don't know how useful implicit solutions are. If you were OK with something implicit, you wouldn't have needed to call solve in the first place. A parameterized solution on the other hand can be quite useful, and is quite different from what you started with.

I do like the idea of always being complex and reminding people to do real=True if they really want that. But that's a separate issue.

Member

asmeurer commented Feb 24, 2014

But it's an implicit solution. I don't know how useful implicit solutions are. If you were OK with something implicit, you wouldn't have needed to call solve in the first place. A parameterized solution on the other hand can be quite useful, and is quite different from what you started with.

I do like the idea of always being complex and reminding people to do real=True if they really want that. But that's a separate issue.

@asmeurer

This comment has been minimized.

Show comment Hide comment
@asmeurer

asmeurer Feb 24, 2014

Member

In other words, if all you want is your equation in terms of re and im, you can just use expand_complex or as_real_imag or .rewrite(re). solve should solve the equation.

Member

asmeurer commented Feb 24, 2014

In other words, if all you want is your equation in terms of re and im, you can just use expand_complex or as_real_imag or .rewrite(re). solve should solve the equation.

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 24, 2014

Member

expand and solve works for this

>>> x = Symbol('x'); eq=abs(x)-3
>>> eq.expand(complex=1)
I*sqrt(re(x)**2 + im(x)**2)*sin(atan2(0, re(x)**2 + im(x)**2)/2) + sqrt(re(x)**2
 + im(x)**2)*cos(atan2(0, re(x)**2 + im(x)**2)/2) - 3
>>> solve(_)
[{re(x): -sqrt(-im(x)**2 + 9), x: -sqrt(-im(x)**2 + 9) + I*im(x)}, {re(x): sqrt(
-im(x)**2 + 9), x: sqrt(-im(x)**2 + 9) + I*im(x)}]
Member

smichr commented Feb 24, 2014

expand and solve works for this

>>> x = Symbol('x'); eq=abs(x)-3
>>> eq.expand(complex=1)
I*sqrt(re(x)**2 + im(x)**2)*sin(atan2(0, re(x)**2 + im(x)**2)/2) + sqrt(re(x)**2
 + im(x)**2)*cos(atan2(0, re(x)**2 + im(x)**2)/2) - 3
>>> solve(_)
[{re(x): -sqrt(-im(x)**2 + 9), x: -sqrt(-im(x)**2 + 9) + I*im(x)}, {re(x): sqrt(
-im(x)**2 + 9), x: sqrt(-im(x)**2 + 9) + I*im(x)}]
@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Feb 24, 2014

Member

An error is now raised for vanilla args to Abs. If tests pass and you like this, go ahead and commit it. I think I'm at the end of modifications.

Member

smichr commented Feb 24, 2014

An error is now raised for vanilla args to Abs. If tests pass and you like this, go ahead and commit it. I think I'm at the end of modifications.

@smichr

This comment has been minimized.

Show comment Hide comment
@smichr

smichr Mar 6, 2014

Member

I'll delete the commented out print statement and commit this tomorrow unless there is an objection.

Member

smichr commented Mar 6, 2014

I'll delete the commented out print statement and commit this tomorrow unless there is an objection.

smichr added a commit that referenced this pull request Mar 6, 2014

Merge pull request #2927 from smichr/abs
issue 4046: better handling of Abs by solve

@smichr smichr merged commit 6f68fa1 into sympy:master Mar 6, 2014

1 check passed

default The Travis CI build passed
Details

@skirpichev skirpichev referenced this pull request Mar 7, 2014

Closed

ToDo after issues migration #7235

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment