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

better _solve_trig, _invert method. #10733

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Shekharrajak
Copy link
Member

Shekharrajak commented Mar 5, 2016

Rewriting
#10552 and
#10482 in effective way with additional changes.

TODO

  • test_solve_lambert
  • test_conditionset_equality
  • test_issue_pow
  • test_solve_only_exp_2

Below fixes is in #10764
otherwise it will be difficult for review and for me as well.

I am trying to make changes, whatever I was thinking since last month.

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented Mar 5, 2016

Oh conflict! seems someone just added domain.

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch 5 times, most recently from 2cc0e05 to 9a5a9db Mar 5, 2016

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch from c68ca43 to 75770f2 Mar 6, 2016

@Shekharrajak Shekharrajak changed the title better _solve_trig better _solve_trig, _invert method. Mar 6, 2016

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch 2 times, most recently from d6bbe89 to a667b29 Mar 6, 2016

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented Mar 6, 2016

def test_solve_only_exp_2_fixed_by_10733():
    assert solveset(sqrt(exp(x)) + sqrt(exp(-x)) - 4, x, S.Reals) == \
    FiniteSet(log(-4*sqrt(3) + 7), log(4*sqrt(3) + 7))

I didn't understand why this build failed for above case.

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented Mar 6, 2016

After the review ,it will be helpful for me to go ahead to try TODO part or not.

@@ -192,6 +191,11 @@ def inv(trig):
invs += Union(*[imageset(Lambda(n, L(g)), S.Integers) for g in g_ys])
return _invert_real(f.args[0], invs, symbol)

if (isinstance(a,log) for a in f.args):

This comment has been minimized.

@jksuom

jksuom Mar 6, 2016

Member

It seems that this is a tuple. A tuple is apparently never false:

>>> if (False,):
...     print 'not false'
... 
not false

This comment has been minimized.

@Shekharrajak

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 6, 2016

Author Member

I felt the same problem. Should I remove the if condition @jksuom ?
It can work without it,I just wanted to check if there is any log present or not ?

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 6, 2016

Author Member

Oh! I should use any.After the build completion I will change this to
any(isinstance(a,log) for a in f.args)

This comment has been minimized.

@smichr

smichr Mar 11, 2016

Member

seems that this is a tuple

Just a point of information, it is a generator -- a lazy list comprehension -- which is always True as you have said.

This comment has been minimized.

@smichr

smichr Mar 11, 2016

Member

wanted to check if there is any log present or not

You can use expr.has(log) to do this, but you should only exclude log that have the symbol of interest, something like if any(symbol in l.free_symbols for l in expr.atoms(log)): (assuming symbol is the symbol being solved for and expr is what you are testing to see if it is log-free or not.

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 14, 2016

Author Member

Thanks @smichr . These lines looks good.I will replace.

@@ -218,7 +222,7 @@ def _invert_complex(f, g_ys, symbol):

if hasattr(f, 'inverse') and \
not isinstance(f, TrigonometricFunction) and \
not isinstance(f, exp):
not isinstance(f, exp) and not isinstance(f, HyperbolicFunction):

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 6, 2016

Author Member

Changes for #9531 #9606

else:
return ConditionSet(symbol, Eq(f_original, 0), S.Reals)
soln = _solveset(f, symbol, S.Complexes)
if isinstance(soln, ConditionSet):

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 6, 2016

Author Member

Previously conditionSet come withexp form of the trig equation. Now if solveset can't solve using rewrite(exp) then try to solve without converting that into exp .
And it can't solve then returns ConditionSet in trigonometricFunction.

return S.EmptySet
else:
return ConditionSet(symbol, Eq(f_original, 0), S.Reals)
soln = _solveset(f, symbol, S.Complexes)

This comment has been minimized.

@Shekharrajak
if isinstance(soln, ConditionSet):
# try to solve without converting it into exp form
# TODO need more improvement here.
soln = _solve_as_poly(f_orig, symbol, domain)

This comment has been minimized.

@Shekharrajak
@@ -725,11 +737,11 @@ def solveset(f, symbol=None, domain=S.Complexes):
but there may be some slight difference:
>>> pprint(solveset(sin(x)/x,x), use_unicode=False)
({2*n*pi | n in Integers()} \ {0}) U ({2*n*pi + pi | n in Integers()} \ {0})
{n*pi | n in Integers()} \ {0}

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 6, 2016

Author Member

because it is changed to simplified expression.

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch from a667b29 to ba4e730 Mar 6, 2016

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented Mar 6, 2016

Ping @asmeurer @hargup @aktech @smichr . Please review any type of comment will be helpful for me.

f_tmp = logcombine(f , force = True)
if isinstance(f_tmp,log):
invs = E**(list(g_ys)[0])
return _invert_real(simplify(f_tmp.args[0]), FiniteSet(invs), symbol)

This comment has been minimized.

@smichr

smichr Mar 11, 2016

Member

it's better to leave simplification to the user. See the docstring of simplify (I believe) for reasons why.

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 14, 2016

Author Member

Actual problem was if we pass without simplifying in _invert_real then because of multiple operators it won't go inside the particular if condition and return unsolved.
There are some more cases where if we pass simplified expression then _invert can give answer for example this testcase

for L in inv(f):
invs += Union(*[imageset(Lambda(n, L(g)), S.Integers) for g in g_ys])
return _invert_real(f.args[0], invs, symbol)
#TODO similarly for HyperbolicFunction

This comment has been minimized.

@smichr

smichr Mar 11, 2016

Member

hyperbolics can be rewritten in terms of trig with the Osborne transformation and should be handled exactly the same way as the trig expressions. Otherwise you start to get diverging behavior in how the two respond to attempts at solution.

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 14, 2016

Author Member

I didn't understand Osborne transformation , Please tell me more about this.

This comment has been minimized.

@smichr

smichr Mar 14, 2016

Member

I believe there is a Wikipedia reference in the docstring of the Osborne functions in fu.py in simplify. Basically, hyperbolics can be written in terms of trigonometric functions with a change of args.

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 14, 2016

Author Member

Nice ! So we don't need separate code for Hyperbolic function just need to call _osborne method from fu.py and replace all the HyperblicFunc above this if statement.
I will add these lines in next commit in _invert_real as well.
Thanks.

@@ -231,6 +235,26 @@ def _invert_complex(f, g_ys, symbol):
for g_y in g_ys if g_y != 0])
return _invert_complex(f.args[0], exp_invs, symbol)

if isinstance(f, TrigonometricFunction):

This comment has been minimized.

@smichr

smichr Mar 11, 2016

Member

Let's move this to _solve_trig -- it's a special case of solving a trig expression: the case when the expression is a trig function. These cases can be solved without rewriting as exp.

This comment has been minimized.

@Shekharrajak

Shekharrajak Mar 14, 2016

Author Member

In _invert_real it is handled similar way. here
So I think it should be here only.

xfail fixed
l194 checking log in expr effective way

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch 2 times, most recently from 5c9b850 to c01bc69 Apr 27, 2016

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented Apr 27, 2016

It seems there is comparison error here
It Works fine locally!

@Shekharrajak

This comment has been minimized.

Copy link
Member Author

Shekharrajak commented May 2, 2016

@aktech , I tried the testcase in Python 3.5 it works fine locally

In [1]: y, a = symbols('y, a')

In [2]:     solveset(sin(y + a) - sin(y), a) == \
        ImageSet(Lambda(n, (-1)**n*asin(sin(y)) + n*pi - y), S.Integers)   ...: 
Out[2]: True

But testcase failed when I did bin/test. I checked using PuDB there also it looks correct.
I think I should SKIP this case as comparison error.

@Shekharrajak Shekharrajak force-pushed the Shekharrajak:solveset_trig branch from 442a9bb to 7964de6 May 25, 2016

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.