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

Added transcendental checks, rewrite exp for power #14712

Merged
merged 10 commits into from Jun 15, 2018

Conversation

Projects
None yet
5 participants
@sidhantnagpal
Member

sidhantnagpal commented May 12, 2018

References to other Issues or PRs

Fixes #14262
Fixes #12744

Brief description of what is fixed or changed

_eval_rewrite_as_exp is added for Pow

Other Comments

Fixed a couple of bugs in solvers with help from @jksuom @smichr
Fixes #14791

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 12, 2018

Looks like an issue with transcendental (travis). Will look into it soon.

if self.base.is_zero or _is_one(self.base):
return False
elif self.base.is_real and self.exp.is_irrational:

This comment has been minimized.

@asmeurer

asmeurer May 12, 2018

Member

According to the theorem, this should be is_algebraic, not is_real.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 13, 2018

Member

Yes, the issue is the theorem happens to use a different meaning for algebraic (including complex values). Here.

return False
elif self.base.is_real and self.exp.is_irrational:
return True
elif self.base.is_transcendental and self.exp.is_zero == False:

This comment has been minimized.

@asmeurer

asmeurer May 12, 2018

Member

This is also wrong. The theorem only applies when the base is algebraic (see https://en.wikipedia.org/wiki/Gelfond%E2%80%93Schneider_theorem, the counterexample (sqrt(2)**sqrt(2))**sqrt(2). This would be true if you added self.exp.is_rational as t**r is transcendental for transcendental t and nonzero rational r.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 13, 2018

Member

Yes, I also figured this, will make the required changes in next round. Thanks.

@@ -1212,6 +1212,22 @@ def _is_one(expr):
or self.base.is_irrational):
return self.exp.is_rational
def _eval_is_transcendental(self):
# Gelfond-Schneider theorem

This comment has been minimized.

@asmeurer

asmeurer May 12, 2018

Member

Isn't this already implemented in _eval_is_algebraic? The assumptions system already knows that transcendental == complex & !algebraic.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 13, 2018

Member

Yes, looks like the remaining values are correct in master.

>> ((S(2)/3 + I*S(7)/8)**sqrt(2)).is_transcendental
True
>> ((1 + sqrt(3))**sqrt(2)).is_transcendental
True

So, I will remove it. Thanks.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 14, 2018

Looks like _tsolve and _solve are leading to recursion depth exceeded.
What could be the reason for it?

@sidhantnagpal

Although a minor issue, but is there a reason the test file was named test_eval_power.py rather than test_power.py.

@asmeurer

This comment has been minimized.

Member

asmeurer commented May 14, 2018

I don't know. You might find the answer if you look through the git history.

@jksuom

This comment has been minimized.

Member

jksuom commented May 14, 2018

The file was created by commit dfc19f5 whose message says: Fixed bug in Rational._eval_power and added preliminary tests for this type. So it seems that initially only _eval_power was tested.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 14, 2018

Okay, if renaming is to be done, I can include it in this PR itself.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 14, 2018

Okay, so the recursion error is due to the line rewrite = lhs.rewrite(exp) in _tsolve (transcendental solve). An alternate might be required for the intended use.

@asmeurer

This comment has been minimized.

Member

asmeurer commented May 16, 2018

It likely indicates a bug in tsolve. You will probably need to step through it to see what it is doing and why it recurses infinitely.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

Yes. There seems to be following issues.
For rewrite:
expand_complex(x) will cause variable expansion to re(x) + I*im(x) (under default assumptions, which can cause problems in solvers where variables should remain atomic).

For _tsolve:
The statement lhs.rewrite(exp) should be rethought on possibly to handle rewrites which change the expression format (the initial intention for writing it may no longer hold).

@asmeurer

This comment has been minimized.

Member

asmeurer commented May 17, 2018

Why is expand_complex used at all? Should it rather be expand_complex(deep=False)?

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

I am thinking of removing expand_complex, as separate functionality already exists:

def _eval_rewrite_as_exp(self, base, expo):
        from sympy import exp, log, I
        if not base.is_zero:
            if base.has(Symbol):
                # delay evaluation if expo is non symbolic
                # (as exp(x*log(5)) automatically reduces to x**5),
                return exp(log(base)*expo) if expo.has(Symbol) \
                    else exp(log(base)*expo, evaluate=False)
            else:
                u, v = log(base).as_real_imag()
                return exp((u + I*v)*expo)

That way, we can have the following:

    expr = 5**(6 + 7*I)
    assert expr.rewrite(exp) == exp((6 + 7*I)*log(5))
    assert expr.rewrite(exp).expand() == 15625*exp(7*I*log(5))

    x, y = symbols('x y')
    assert (x**y).rewrite(exp) == exp(y*log(x))
    assert (7**x).rewrite(exp) == exp(x*log(7), evaluate=False)
    assert ((2 + 3*I)**x).rewrite(exp) == exp(x*(log(sqrt(13)) + I*atan(S(3)/2)))
@asmeurer

This comment has been minimized.

Member

asmeurer commented May 17, 2018

I see why it is there now. I think deep=False is what you want. Or you could just hard-code it as expo*(log(abs(base)) + arg(base)*I).

@asmeurer

This comment has been minimized.

Member

asmeurer commented May 17, 2018

In fact I would hard-code it, since you can also just precompute the exp(log(abs(base))*expo) reduction, and avoid unnecessary computation to do the rewrite.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

I thought about using deep=False, but unfortunately it still expands variables

>>> x = Symbol('x')
>>> expand_complex(x, deep=False)
re(x) + I*im(x)
@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

In fact I would hard-code it

Yes, I have hard coded it as used in the snippet here.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

Regarding the solvers issue:
For the test solve(3*x + 5 + 2**(-5*x + 3), x):
(adding print(lhs, rhs, rewrite, sep=' | ') after rewrite = lhs.rewrite(exp))

The previous results are:

2**(-5*x + 3) + 3*x | -5 | 2**(-5*x + 3) + 3*x
32**_x*(3*_x + 5) | -8 | 32**_x*(3*_x + 5)

The current results are:

2**(-5*x + 3) + 3*x | -5 | 3*x + exp((-5*x + 3)*log(2))
2**(-5*x + 3) + 3*x | -5 | 3*x + exp((-5*x + 3)*log(2))
2**(-5*x + 3) + 3*x | -5 | 3*x + exp((-5*x + 3)*log(2))
... (infinite times)

What exactly does the rewrite line do?

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

The tests affected by lhs.rewrite(exp) are:
solve(sin(x) + tan(x)) and solve(sin(3*x) + sin(6*x)).

@jksuom

This comment has been minimized.

Member

jksuom commented May 17, 2018

It seems that test_issue_14640 repeatedly tries to solve (or _tsolve) the same equation. There is a flag, flags['tsolve_saw'] that collects those equations that have been examined, but that is not passed on here.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

Yes, this fixes the infinite recursion.
Although, there are still failing tests due to the rewrite (as more equations are now in the scope of lhs!=rewrite). I will push the changes for further review.

sidhantnagpal added some commits May 17, 2018

# (as exp(x*log(5)) automatically reduces to x**5)
return exp(log(base)*expo, evaluate=expo.has(Symbol))
else:
u, v = log(base).as_real_imag()

This comment has been minimized.

@asmeurer

asmeurer May 17, 2018

Member

I would still do deep=False here, or just use log(abs(base)) + arg(base)*I. rewrite shouldn't do deep changes to the expression that are unrelated to the function it is rewriting to.

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 17, 2018

Member

Okay, will make the changes along with the remaining tests.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 18, 2018

I tried to trace the execution for the failing tests. Turns out all of them are related to solvers.
The failing tests are:

  1. test_util.py:
    function_range(tan(x), x, Interval(0, pi))
    continuous_domain(tan(x), x, Interval(0, 2*pi))
  2. test_inequalities.py:
    solve_univariate_inequality(tan(x) < S.One, x, relational=False)
  3. test_solvers.py:
    solve(3*log(a**(3*x + 5)) + a**(3*x + 5), x)
    solve(sin(x) + tan(x))

I am not that versed in solvers. It would be better if someone could help with these tests.
Meanwhile, the changes to power.py are almost complete.

@jksuom

This comment has been minimized.

Member

jksuom commented May 18, 2018

solveset will use rewrite(exp) for trigonometric functions. When trying to find the zeros of 1/tan(x) it will get

>>> (1/tan(x)).rewrite(exp)
exp(-log(I*(-exp(I*x) + exp(-I*x))/(exp(I*x) + exp(-I*x))))

instead of

>>> (1/tan(x)).rewrite(exp)
-I*(exp(I*x) + exp(-I*x))/(-exp(I*x) + exp(-I*x))

Apparently it cannot solve the modified form and returns:

>>> solveset(1/tan(x), x)
EmptySet()

instead of

>>> solveset(1/tan(x), x)
Union(ImageSet(Lambda(_n, 2*_n*pi + pi/2), S.Integers), ImageSet(Lambda(_n, 2*_n*pi + 3*pi/2), S.Integers))
@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 18, 2018

Also, in current master:
solve(exp(log(5)*x) - 2**x, x) gives [0]
but
solve(exp(log(5)*x) - exp(log(2)*x), x) gives [].

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 18, 2018

solveset will use rewrite(exp) for trigonometric functions.

Okay, so the issue is caused by 1/tan(x) (and similar) being an instance of Pow (and hence rewrite affecting how they are solved).
Maybe we can check if argument arg in exp(log(arg)) or exp(-log(arg)) already contains exp, or perform conversion of 1/tan to cot so that native rewrite is used.
I think solvers should be able to handle both rewrites, ideally.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 30, 2018

test_util and test_inequalities are now fixed.

test_solvers still has the following failing tests:
solve(exp(log(5)*x) - 2**x, x) and solve(3*log(a**(3*x + 5)) + a**(3*x + 5), x).

@smichr

This comment has been minimized.

Member

smichr commented Jun 12, 2018

solve(exp(log(5)*x) - exp(log(2)*x), x) gives []

This gives [0] if x is declared as real.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented Jun 12, 2018

Yes, that helps, but it would be good to have so under default assumptions (without knowing the nature of solution).

@smichr

This comment has been minimized.

Member

smichr commented Jun 12, 2018

There is a genuine problem in the code. Let me see if I can do a pr against this branch.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented Jun 13, 2018

Sure, that'd be great.

@smichr

This comment has been minimized.

Member

smichr commented Jun 13, 2018

OK, I sent the PR. A good side effect is that all 3 solutions are now given for a generic variable (you will see the modified test in the test file).

@smichr

This comment has been minimized.

Member

smichr commented Jun 13, 2018

You can see the PR here

smichr added some commits Jun 13, 2018

handle f(x) = f(1) sort of cases
The upper code handles cases where the two functions
share a symbol of interest; the lower handles the
case where they don't.
@smichr

This comment has been minimized.

Member

smichr commented Jun 15, 2018

Until you press the Commit button on the PR that I sent, the changes won't appear in this branch.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented Jun 15, 2018

Yes, everything is working fine. Merging it. Thanks.

Merge pull request #1 from smichr/power-exp
fix _invert and modify tests
@smichr

This comment has been minimized.

Member

smichr commented Jun 15, 2018

it's in

@smichr smichr merged commit 52f238f into sympy:master Jun 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented Jun 16, 2018

Great, this also closes #14791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment