Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

assumptions modifications for imaginary bases #2952

Merged
merged 11 commits into from Mar 9, 2014

Conversation

Projects
None yet
3 participants
Member

smichr commented Feb 22, 2014

edit assumptions

add duplicate test to code quality checking

Contributor

akshayah3 commented Feb 23, 2014

Consider the following:
I**I (I.pow(I)).It is positive and it's value is exp(-pi/2) but these kind of numbers are not supported in the power class and return none for every method like the functions you added
eval_is_nonpositive and eval_is_nonnegative

Member

smichr commented Feb 23, 2014

I added that handling, too, now.

Contributor

akshayah3 commented Feb 23, 2014

I**im handling looks good.

@smichr smichr commented on the diff Feb 23, 2014

sympy/core/facts.py
@@ -458,6 +458,10 @@ class FactKB(dict):
"""
A simple propositional knowledge base relying on compiled inference rules.
"""
+ def __str__(self):
@smichr

smichr Feb 23, 2014

Member

It's really annoying to try work with an unsorted large dictionary when trying to see what different key,value pairs are, so I added this.

@asmeurer asmeurer commented on an outdated diff Feb 23, 2014

sympy/core/power.py
@@ -263,6 +263,19 @@ def _eval_is_negative(self):
if self.exp.is_even:
return False
+ def _eval_is_nonpositive(self):
+ if self.base.is_imaginary:
+ if self.exp.is_Integer and self.exp.is_even:
@asmeurer

asmeurer Feb 23, 2014

Owner

Why does it have to be an Integer?

@asmeurer asmeurer commented on an outdated diff Feb 23, 2014

sympy/core/power.py
@@ -263,6 +263,19 @@ def _eval_is_negative(self):
if self.exp.is_even:
return False
+ def _eval_is_nonpositive(self):
+ if self.base.is_imaginary:
+ if self.exp.is_Integer and self.exp.is_even:
+ return True
+
+
+ def _eval_is_nonnegative(self):
+ if self.exp.is_Integer and self.exp.is_even and self.base.is_real:
@asmeurer

asmeurer Feb 23, 2014

Owner

Same here. What about r**n where r = Symbol('r', real=True) and n = Symbol('n', even=True)?

Owner

asmeurer commented Feb 23, 2014

A little more general would be log(base).is_imaginary. The reason I**I is real is that it's the same as exp(log(I**I) = exp(I*log(I)). log(I) is I*pi/2, which is pure imaginary, so the Is cancel. The same works for (-1)**I for the same reason, because log(-1) is I*pi. So generally, x**y is real if log(x).is_imaginary and y.is_imaginary.

Owner

asmeurer commented Feb 23, 2014

As for positive or negative, exp(real) is always positive. So it's actually x**y is positive under those conditions.

Member

smichr commented Feb 24, 2014

Everything was made a bit more general. These are always hairy modifications, so a close look by others would be appreciated. If you're convinced it's right, please commit. Otherwise, feel free to close this and modify as you see fit.

@smichr smichr closed this Feb 24, 2014

@smichr smichr deleted the smichr:reim branch Feb 24, 2014

@smichr smichr reopened this Feb 24, 2014

@smichr smichr commented on an outdated diff Feb 26, 2014

sympy/assumptions/handlers/order.py
@@ -216,7 +216,7 @@ def Add(expr, assumptions):
@staticmethod
def Pow(expr, assumptions):
if expr.is_number:
- return expr.evalf() > 0
+ return expr.evalf().is_positive
@smichr

smichr Feb 26, 2014

Member

I added a test in test_pow_im_base that shows why this is necessary

Owner

asmeurer commented Feb 27, 2014

These are always hairy modifications, so a close look by others would be appreciated.

That's something I'm hoping my work with #2508 will fix. With it, inconsistent assumptions raise an exception, so as long as your test cases cover everything, you can be fairly confident. It's already caught me a few times trying to add invalid facts because I had the math wrong in my head.

@asmeurer asmeurer commented on an outdated diff Feb 27, 2014

sympy/assumptions/handlers/sets.py
@@ -451,6 +462,17 @@ def Pow(expr, assumptions):
return False
elif ask(Q.negative(expr.base), assumptions):
return True
+ elif ask(Q.imaginary(expr.exp), assumptions):
+ im_b = ask(Q.imaginary(C.log(expr.base)), assumptions)
+ return fuzzy_not(im_b)
+
+ @staticmethod
+ def log(expr, assumptions):
+ arg = expr.args[0]
+ if arg.func == C.exp:
+ return ask(Q.imaginary(arg.args[0]), assumptions)
@asmeurer

asmeurer Feb 27, 2014

Owner

I think this is OK. log(exp(x)) != x, it equals x + 2_pi_I_n for whatever integer n give an imaginary part between 0 and 2_pi*I. But adding an imaginary value won't change what's imaginary.

Ahh, but there may be an issue with the fact that we don't consider 0 to be imaginary. log(exp(x)) = 0 when x = 2_pi_I, which is imaginary. IMHO this is a pretty good argument for making 0 imaginary. Otherwise, we have to check if x is an integer multiple of 2_pi_I, which is not easy to do in the assumptions system, even the new one.

@asmeurer asmeurer and 1 other commented on an outdated diff Feb 27, 2014

sympy/assumptions/handlers/sets.py
@@ -451,6 +462,17 @@ def Pow(expr, assumptions):
return False
elif ask(Q.negative(expr.base), assumptions):
return True
+ elif ask(Q.imaginary(expr.exp), assumptions):
+ im_b = ask(Q.imaginary(C.log(expr.base)), assumptions)
+ return fuzzy_not(im_b)
+
+ @staticmethod
+ def log(expr, assumptions):
+ arg = expr.args[0]
+ if arg.func == C.exp:
+ return ask(Q.imaginary(arg.args[0]), assumptions)
+ if arg.is_number and ask(Q.complex(arg), assumptions):
@asmeurer

asmeurer Feb 27, 2014

Owner

Why this? All numbers are complex. You're basically saying log is never imaginary here.

@smichr

smichr Feb 27, 2014

Member

I've had in my mind real ->2, imaginary-2*I, complex -> 2 + 2*I; I see you are right so this needs to be changed. I'm disappointed not to have seen that this was untested (or that I missed the test that is wrong) since I checked coverage.

@smichr

smichr Feb 27, 2014

Member

This is part of what makes this whole business tricky: I originally had a test that arg not be in [I,-I] since that leads to an imaginary result but the assumption system is able to resolve this without that code there:

>>> ask(Q.imaginary(log(I)))
True
>>> log(I).n()
1.5707963267949*I

The test should be (I think) if arg.is_number and arg not in [I, -I]: return False

@asmeurer

asmeurer Mar 2, 2014

Owner

Code coverage is not enough with the assumptions. You have to do a logical coverage, meaning check that all combinations of logical conditions are covered. Since if x: is covered whether x is true or false, and if x or y: is covered even if y is never evaluated, you need to do more (if x or y needs to check four combinations of x and y being true and false; for the assumptions, we should be checking all combinations of True, False, and None).

This is actually pretty a typical feature of more robust coverage tools, for instance, coverage of mission critical code like the code that runs in an airplane. I wonder if Python has any more advanced coverage tools, or if coverage has the option to do this sort of thing.

@asmeurer asmeurer commented on an outdated diff Feb 27, 2014

sympy/core/mod.py
@@ -35,7 +35,9 @@ def doit(p, q):
to be less than q.
"""
- if p == q or p == -q or p.is_Pow and p.exp.is_Integer and p.base == q:
+ if (p == q or p == -q or
+ p.is_Pow and p.exp.is_Integer and p.base == q or
+ p.is_integer and q == 1):
@asmeurer

asmeurer Feb 27, 2014

Owner

Looks good.

@asmeurer asmeurer and 1 other commented on an outdated diff Feb 27, 2014

sympy/core/power.py
@@ -243,6 +243,15 @@ def _eval_is_positive(self):
elif self.base.is_nonpositive:
if self.exp.is_odd:
return False
+ elif self.base.is_imaginary:
+ if self.exp.is_integer:
+ m = self.exp % 4
+ if m == 0:
@asmeurer

asmeurer Feb 27, 2014

Owner

When will this evaluate to 0 but the power not auto-evaluate in the first place? Shouldn't we use .is_zero instead?

@smichr

smichr Feb 27, 2014

Member
>>> i = var('i', imaginary=1)
>>> i**4
i**4

Yes, I can use is_zero.

@asmeurer asmeurer and 1 other commented on an outdated diff Feb 27, 2014

sympy/core/power.py
@@ -243,6 +243,15 @@ def _eval_is_positive(self):
elif self.base.is_nonpositive:
if self.exp.is_odd:
return False
+ elif self.base.is_imaginary:
+ if self.exp.is_integer:
+ m = self.exp % 4
+ if m == 0:
+ return True
+ if m.is_Integer:
@asmeurer

asmeurer Feb 27, 2014

Owner

is_integer. You should use the assumptions inside of the assumptions.

@asmeurer

asmeurer Feb 27, 2014

Owner

And of course this should only be called if is_zero is explicitly False, not None.

@smichr

smichr Feb 27, 2014

Member

Right -- that's why I am using is_Integer. If the m is Mod() then it was unevaluated and not an Integer so I can't conclude anything. If the assumption system is smart it will tell me that Mod(integer, 4) is an integer but I don't know the value so I shouldn't conclude. Another way of saying this is if m in [1, 2, 3]: -- maybe that would be better?

@asmeurer

asmeurer Mar 2, 2014

Owner

I was thinking something like

if m.is_zero:
    return True
elif m.is_zero is False:
    if m.is_integer:
        ...
else: # m.is_zero is None, can't conclude anything
    ...

@asmeurer asmeurer commented on the diff Feb 27, 2014

sympy/core/power.py
@@ -288,6 +297,8 @@ def _eval_is_integer(self):
def _eval_is_real(self):
real_b = self.base.is_real
if real_b is None:
+ if self.base.func == C.exp and self.base.args[0].is_imaginary:
+ return self.exp.is_imaginary
@asmeurer

asmeurer Feb 27, 2014

Owner

There's the same issue here with 0 not being imaginary.

@asmeurer asmeurer commented on the diff Feb 27, 2014

sympy/core/power.py
re, im = self.base.as_real_imag(deep=deep)
+
+ if im.is_zero and self.exp is S.Half:
+ if re.is_nonnegative:
+ return self, S.Zero
+ if re.is_nonpositive:
+ return S.Zero, (-self.base)**self.exp
@asmeurer

asmeurer Feb 27, 2014

Owner

This seems like a very special case, but it looks correct.

@asmeurer asmeurer commented on an outdated diff Feb 27, 2014

sympy/functions/elementary/exponential.py
@@ -659,7 +659,14 @@ def _eval_is_rational(self):
return s.is_rational
def _eval_is_real(self):
- return self.args[0].is_positive
+ if self.args[0].is_real:
+ return self.args[0].is_nonnegative
+ if self.args[0].is_complex:
+ return False
+
+ def _eval_is_imaginary(self):
+ if self.args[0].func == C.exp:
+ return self.args[0].args[0].is_imaginary
@asmeurer

asmeurer Feb 27, 2014

Owner

And here again.

Owner

asmeurer commented Feb 27, 2014

OK, I carefully reviewed everything. It seems this will be a little limited by the fact that 0 is not considered to be imaginary (and there's no way in the old assumptions to say "imaginary or zero").

@smichr smichr closed this Feb 27, 2014

@smichr smichr reopened this Feb 27, 2014

Member

smichr commented Feb 27, 2014

Thanks for the comments, @asmeurer . I've reworked things and left the new changes as a separate commit to make review easier when you get a chance to look at this again.

Member

smichr commented Feb 28, 2014

@ness01 , I'm running into a problem with hyperexpand's formulation after allowing Mod to recognize that Mod(k + 1, 1) == 0 if k is an integer. The compute_buckets routine formerly returned things like Mod(k, 1) for Mod(k + 1, 1) but now it is returning 0; the symbolic value that get stored with 0 (e.g. the k) causes problems later when the list containing that k along with non-symbolic values gets sorted:

>>> l=[i,i+1]
>>> l.sort()
>>> l
[i, i + 1]
>>> l=[i,0]
>>> l.sort()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy\core\relational.py", line 111, in __nonzero__
    raise TypeError("symbolic boolean expression has no truth value.")
TypeError: symbolic boolean expression has no truth value.

What I don't know is what should happen: perhaps we could use a sorting key

>>> list(ordered([i,0]))
[0, i]

Or maybe a different key should be used to identify buckets: instead of using Mod(x, 1) we could use x.as_coeff_Add()[1]?

If you look at the "watch for TypeError"-commit here you can see how I was trying to work around the problem and the results that were obtained. I'm going to try the coeff_Add approach to see if I can break less existing expectations (it's my reim2 branch and the tests are running now https://travis-ci.org/smichr/sympy/builds/19833164). If you have any suggestions, they would be appreciated.

Member

smichr commented Mar 1, 2014

@ness01 , @asmeurer I think I have the hyperexpand issue worked around with a slightly modified mod1 function that gets at the essence of what was being done previously.

In the process of fixing that, other small commits were made so the last 6 commits need a look over; actually, to make things easier I will split out those commits.

Member

smichr commented Mar 1, 2014

This is waiting for a final signoff. The other work is in 3 different pull requests.

Owner

asmeurer commented Mar 2, 2014

It's just the most recent commit that I need to look at, right?

Owner

asmeurer commented Mar 2, 2014

Assuming so, it looks fine. I didn't check the test coverage, so I'll leave that to you.

Member

smichr commented Mar 4, 2014

This has been a real slog. If you could look at the last 5 commits, @asmeurer , that would be appreciated. They can be reviewed independently of each other since each basically addresses a different concern. "minor edits", for example, just deals with cosmetic issues. I'll check the coverage again, too.

BTW, one disturbing thing I notice is that core tests cover a small percentage of the code; the actual coverage is much higher when other non-core code uses the core. It seems like the tests for a given module should address coverage or else when non-core (for example) tests are changed you could be changing the coverage of the codebase.

Member

smichr commented Mar 5, 2014

Everything is covered except for the printing of the assumptions dictionary (which I tested locally). So if the tests look good this should be ready.

smichr added some commits Mar 5, 2014

unXFAIL ask(Q.real(NaN)) is None tests
Note: the old assumptions return None for queries about NaN whereas
the new assumptions (because NaN gets handled by _number) returns
False -- if it is important that this return None, the following can
be added to the pertinent handlers:

    NaN = staticmethod(lambda x, y: None)
Member

smichr commented Mar 5, 2014

These are tests that fail in master that pass here:

i = Symbol('i', imaginary=True)
x = Symbol('x')

assert sqrt(i**2).as_real_imag() == (0, sqrt(-i**2))

assert (I**i).is_real is True
assert ((-I)**i).is_real is True
assert (exp(i)**I).is_real is True
assert (i**2).is_nonpositive is True
assert (i**4).is_nonpositive is False
assert (I**i).is_nonnegative is True

assert ask(Q.imaginary(I**x), Q.imaginary(x)) is False
assert ask(Q.imaginary((2*I)**x), Q.imaginary(x)) is False
assert ask(Q.imaginary(log(I + 1))) is False
assert ask(Q.imaginary(log(exp(x))), Q.imaginary(x)) is None  # zoo/I/a+I*b
assert ask(Q.imaginary(log(exp(I)))) is True
assert ask(Q.imaginary(exp(x)**x), Q.imaginary(x)) is False
eq = Pow(exp(pi*I*x, evaluate=False), x, evaluate=False)
assert ask(Q.imaginary(eq), Q.even(x)) is False
eq = Pow(exp(pi*I*x/2, evaluate=False), x, evaluate=False)
assert ask(Q.imaginary(eq), Q.odd(x)) is True
assert ask(Q.imaginary(exp(3*I*pi*x)**x), Q.integer(x)) is False
assert ask(Q.imaginary(exp(2*pi*I, evaluate=False))) is False
assert ask(Q.imaginary(exp(pi*I/2, evaluate=False))) is True
assert ask(Q.positive(2**I)) is False
assert ask(Q.positive(cos(I)**2 + sin(I)**2 - 1)) is None
assert ask(Q.positive(-I + I*(cos(2)**2 + sin(2)**2))) is None
assert ask(Q.positive(log(x)), Q.imaginary(x)) is False
assert ask(Q.positive(log(x)), Q.negative(x)) is False
assert ask(Q.real((-I)**i), Q.imaginary(i)) is True
assert ask(Q.real(I**i), Q.imaginary(i)) is True
assert ask(Q.real(exp(2*pi*I, evaluate=False))) is True
assert ask(Q.real(exp(pi*I/2, evaluate=False))) is False
assert ask(Q.real(log(I + 1))) is False
assert ask(Q.real(log(x)), Q.imaginary(x)) is False
eq = Pow(exp(2*pi*I*x, evaluate=False), x, evaluate=False)
assert ask(Q.real(eq), Q.integer(x)) is True

smichr added some commits Mar 5, 2014

modify AskPositiveHandler.log
Also changed ~negative to nonnegative since ~negative does
not imply that the number is even real: it could be imaginary
or positive or zero. nonnegative implies that the number is
real and is not 0 or negative (and that's what the test in Add
is looking for).
Member

smichr commented Mar 5, 2014

I ran into problems where a test suite was not being tested because it was a duplicate. I now added a test for such duplicates in code_quality and fixed the many files in which this was happening.

fix AskPositiveHandler.Pow
There was a duplicate .even test; the second one should have
been odd. It was not caught the last time changes were made
to these lines, probably because the change was automated. I
found the need for a change because this was a line that was
uncovered with tests.

Other tests were also added to bring the coverage in `sets` almost
to 100% (the handling in _number is not complete but may represent
lines that will be unreachable/unlikely). In the `order` file there
is a significant amount of code for the hermitian property that
is not covered.
Owner

asmeurer commented Mar 8, 2014

assert ask(Q.positive(-I + I_(cos(2)__2 + sin(2)_*2))) is None

That's impressive that that works. How is it doing it?

Why 2*i? Shouldn't it be i/2?

Owner

smichr replied Mar 8, 2014

If the arg is an integer or half-integer multiple of I*pi then doubling will make either case be an integer. In one case the base is equivalent to +/-1 and in the other, +/-I. If it is +/-1 then i is an integer and (-1)**i will be real; if it is +/-I then i is a half-integer and (-1)**i will be +/-I. The overall realness of the original expression will then be determined by the base's value (+/-1 or +/-I) raised to expr.exp.

You should add this as a comment here.

Owner

smichr replied Mar 9, 2014

done

Where does the -1 come from?

Owner

smichr replied Mar 8, 2014

see note above

Member

smichr commented Mar 8, 2014

how does it work?

Any number is evaluated; the only thing I did is be sure not to conclude unless the number is evaluated with significance. It would be better if this result could say that the answer is False. That could be done with simplify in this case but simplify is not used for speed reasons.

This evaluation-for-decision, however, can still lead to dicey results if, for example, you don't separate real and imaginary parts and just do a blind evaluation. You might get real/imag parts with significance that are 1e-47 in value making the wrong conclusion that that part of the number is non-zero. With my modifications, the whole process is more conserative. I hope this doesn't get lost along the way -- it's easy to think that numbers that are "really small" can be assumed to be zero, but you can't forget about the corner cases where the non-zero number really is small. A lot of this has been thought out in round and int. (I wouldn't be surprised if there are still places for improvement.)

nonpositive means real and not positive.

If you want not positive, just use ~Q.positive

I discussed this in my unfinished work at sympy#2538.

Owner

smichr replied Mar 9, 2014

I think nonpositive is what is needed here since this will give a real value for log.

Owner

asmeurer commented Mar 8, 2014

Sorry for commenting on the commits instead of the pull request diff, but that's the easiest way to review the new changes here. As long as you don't rebase, they won't disappear.

Owner

asmeurer commented Mar 8, 2014

There are good changes here. I'm assuming you are making these changes in order to get something else to work, but it would be nice if you helped collaborate on my work at #2508. I think you'll find the way things work in that branch to be much cleaner. Instead of adding

    @staticmethod
    def log(expr, assumptions):
        return ask(Q.positive(expr.args[0]), assumptions)

to AskRealHandler, you would just add (log, Equivalent(Q.real, AllArgs(Q.positive)) to the list of facts. This says, "for log, being real is equivalent to all the args being positive" (log only has one arg, so "all args" just means "the arg" in this case).

Also, in my branch, any deductions that can be made from the existing facts are made, meaning you only have to add new facts if they actually are new logical information, and, perhaps most importantly, the system checks for inconsistencies automatically, meaning if you make a logical typo or a mathematical error, it will notice and fail.

Owner

asmeurer commented Mar 8, 2014

Anyhow, I have just those comments. I didn't review the floating point stuff too closely, but I trust you know what you're doing there.

Member

smichr commented Mar 9, 2014

Regarding ~Q.positive...that's the wrong assumption to send for the test of AskPositiveHandler.Add routine since it implies that the argument could be imaginary and that will automatically return a False from the routine.

Regarding use for something else -- it started small with the issue that akshayah3 raised. This is one of those things that keeps opening other issues (and even now I found another issue, but I'll open an issue for someone else). Your system sounds like it has better potential. As I worked on this I kept thinking that having a logic function be written on the basis of known inputs somehow would be better than trying to do this by hand.

I'm interested in what you are doing but need to step back for a while. I'll commit what I have here as it extends functionality and adds coverage.

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

Merge pull request #2952 from smichr/reim
assumptions modifications for imaginary bases

@smichr smichr merged commit 8abe404 into sympy:master Mar 9, 2014

Owner

asmeurer commented Mar 9, 2014

Great. I'd love if you could play around with my branch and tell me what you think. And if you have any thoughts on how to speed things up, that would be great too. That branch has really stretched the limits of my knowledge on how to profile Python.

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