added functionality to Q.real and Q.imaginary and fixed issue:2324 #1156

sachin004 · 2012-03-21T16:33:17Z

>>> ask(Q.real(x**3),Q.imaginary(x)&Q.real(2))
False
>>> ask(Q.real(x**2),Q.imaginary(x)&Q.real(2))
True
>>> ask(Q.imaginary(x**3),Q.imaginary(x)&Q.real(2))
True
>>> ask(Q.imaginary(x**2),Q.imaginary(x)&Q.real(2))
False

probably a fix to the issue:2324

mrocklin · 2012-03-21T16:50:46Z

AUTHORS

@@ -192,4 +192,3 @@ M R Bharath <catchmrbharath@gmail.com>
 Matthias Toews <mat.toews@googlemail.com>
 Jorge E. Cardona <jorgeecardona@gmail.com>
 Sanket Agarwal <sanket@sanketagarwal.com>
-Manoj Babu K. <manoj.babu2378@gmail.com>


Why remove Manoj from the authors list?

mrocklin · 2012-03-21T17:02:08Z

In general you're on the right track. You've found where you need to make changes, we'll just need to try again before we get it right.

sachin004 · 2012-03-21T17:41:31Z

Ok , thanks for the suggestions, will work on that and make necessary changes

sachin004 · 2012-03-21T18:06:33Z

@mrocklin made necessary changes to the code

flacjacket · 2012-03-21T18:20:51Z

sympy/assumptions/handlers/sets.py

+                return True
+            else:
+                return False
+        elif(ask(Q.imaginary(expr.args[1]) & ask(Q.imaginary(expr.args[0])):


You're missing 2 closed parentheses.

You should also use and instead of &. If you do something like ask(Q.real(x**y), Q.imaginary(x)), this will raise an error.

Also, if you want to do these ask's, you need to include the assumptions argument.

flacjacket · 2012-03-21T18:21:31Z

You should implement new tests for all the new behavior you are implementing.

flacjacket · 2012-03-21T18:28:19Z

sympy/assumptions/handlers/sets.py

@@ -238,6 +238,8 @@ def Pow(expr, assumptions):
            elif ask(Q.real(expr.exp), assumptions):
                if ask(Q.positive(expr.base), assumptions):
                    return True
+        elif ask(Q.imaginary(expr.base), assumptions):
+            return not ask(Q.imaginary(expr))


I don't think this works, in the case when this ask returns None, you should not return True, which happens if you do ask(Q.real(x**y), Q.imaginary(x)) (after you fix the other 2 comments I made on line 422).

flacjacket · 2012-03-21T18:36:40Z

You should be sure to run the tests on your machine before pushing. You need to fix the parentheses before the tests in assumptions will run, but when you do, there are a couple failures of current tests. There is also trailing whitespace in the file, causing a failure in test_code_quality.

flacjacket · 2012-03-21T18:55:36Z

sympy/assumptions/handlers/sets.py

+                                    if integer%2 ==1
+        Imaginary**integer      -> real
+                                    if integer%2 ==0     
+        Imaginary**Imaginary -> Real


This isn't true in general. I**I is real, but you can change the base and get a complex number, e.g. (3*I)**I.

flacjacket · 2012-03-22T02:12:39Z

So in addition to what I said in the code, there are still several cases that just aren't handled correctly. For example, ask(Q.imaginary(I**y), Q.integer(y)) gives False, ask(Q.imaginary(3**y), Q.integer(y)) gives None, etc.

This leads me to my next point that you must implement new tests whenever you implement new functionality. As I've noted there are many cases that are still wrong, but you have to have a test suite to see that. The tests should cover every corner case that you cover in your code, both numerically and symbolically.

In addition to writing tests, you have to be sure you run the tests and they all pass. There are several test failures, not only the ones I mentioned w.r.t. white space, but also some of the current tests in the assumptions module fail. You have to be sure everything passes before we can commit it.

I'm sorry if this is coming off as overly harsh. The assumption stuff has to be approached very carefully and the logic has to be thoroughly tested not to give a wrong answer for any corner case. If you want to do a GSoC, we also need to make sure you can run the tests and check the new features you are implementing. You are moving in the right direction, and you're doing great for a head first jump into the assumptions module, there are just some key subtleties to work out and a new workflow to adapt to.

mrocklin · 2012-03-22T02:38:14Z

I'll echo the "overly harsh" thing.

Your first pull request will be very difficult. There will be a lot of negative feedback. There are a lot of things every student needs to learn. This is normal.

flacjacket · 2012-03-23T02:00:39Z

sympy/assumptions/handlers/sets.py

@@ -224,6 +224,7 @@ def Pow(expr, assumptions):
        Positive**Real        -> Real
        Real**(Integer/Even)  -> Real if base is nonnegative
        Real**(Integer/Odd)   -> Real
+        Real**Imaginary       -> Imaginary if real is positive


This is wrong. Just knowing a real number is raised to an imaginary number, you can't say anything about the result. That said, your code does not check for this case.

jrioux · 2012-10-26T17:31:40Z

This might interest @mrocklin .

sachin004 · 2012-10-26T17:34:21Z

@flacjacket @certik @mrocklin Sorry for the long delay, I fixed all the issues mentioned by flacjacket and some other, if any other issues let me know.

flacjacket · 2012-10-26T20:39:52Z

I think this looks good now, @sachin004 thanks for finishing this up, regardless of the delay. If nobody else has any comments, this can be merged.

mrocklin · 2012-10-26T21:44:47Z

After a brief glance it looks fine to me.

smichr · 2012-10-27T08:10:10Z

sympy/assumptions/handlers/sets.py

@@ -191,7 +192,7 @@ class AskRealHandler(CommonHandler):

    @staticmethod
    def _number(expr, assumptions):
-        return not expr.as_real_imag()[1]
+        return not expr.as_real_imag()[1].evalf()


Why was this change made? Can you evalf to a lower precision, say 2, and still get the answer?

@smichr sorry for the late response, could you please explain what you mean by lower precision. Just to explain why I have added evalf is for the case of ask(Q.real((-1)**I))

are you just seeing if the imaginary part is zero or not? might .evalf(2) work? evalf() will compute to 15 digits, or something like that. So if the answer involves a complicated imaginary part there is no need to calculate it to high precision just to learn that it is not zero.

yes I am checking whether its imaginary or not, sure I will reduce the precision to 2.

Something you have to be careful of is expressions that may not be zero but have no significance in their evaluation. It's been a while, but if you look at my work in round or int (and commits added around that time by me) you will see me returning None when the answer is calculated without precision. I think I put some notes in the commit messages about that. Have you used gitk to browse the commit log? It's really nice.

I think so. Commits dealing with equals, too.

Here's a pertinent piece of code:

@property def is_comparable(self): is_real = self.is_real if is_real is False: return False is_number = self.is_number if is_number is False: return False if is_real and is_number: return True n, i = self.evalf(2).as_real_imag() if not i.is_Number or not n.is_Number: return False if i: if i._prec != 1: return False return n._prec != 1

A problem with this, however, is that for q=(-1)**I q.is_real comes back as False. That's a bug. Otherwise, the idea from the n, i = part that tries to be careful about reaching a conclusion. If the precision is 1 then you don't know what the number is and in the assumption system, you should return None in that case, not True or False as is done in the above.

You asked whether it makes sense to return None...not if you can figure out what sort of number you are working with. But not all expressions that are zero can be, with a given piece of code, be proved to be zero. And we shouldn't use a numerical approximation to guess that it is zero. It might just be an expression with a very small value. In the code above, if we know something is a number, we try to evaluate it to 2 digits of precision. But if we only get 1 -- and it doesn't matter if we had attempted more, we would still get 1 -- it's because the evaluation system couldn't be sure that that value was or wasn't zero. For most non-pathological cases, the above is going to return True or False. It's going to have trouble when it gets an expression where the imaginary part is a zero that can't be proved to be so.

Thanks for the information, I will go through my code and merge the pull made by you, test it and will get back to you.

btw, issue 3068 has some examples where difficulties in evalf'ing are encountered.

http://code.google.com/p/sympy/issues/detail?id=3068

jrioux · 2012-11-19T21:43:00Z

SymPy Bot Summary: ✳️ Passed after merging sachin004/sachin1 (9a57cd2) into master (211f0f7).
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass

jrioux · 2012-11-24T11:49:21Z

@sachin004, please ping when you get around to address @smichr's comments.

sachin004 · 2012-11-24T13:52:30Z

@jrioux sorry for the delay caught in the middle of an internship, I have addresses few of the comments by @smichr for the remaining I have added the test cases as soon as I receive updates from @smichr I will commit the changes.

3532: Pow.is_real fix for base = -1

Expressions may give a clear 0 for the imaginary part if they are allowed to simplify through as_real_imag but if evalf is done first then a _prec=1 result is obtained: >>> I**(I+2)).n(2) -0.21 + 0.e-8*I >>> _.as_real_imag()[1] 0.e-8 >>> _._prec 1 That "1" means there was no precision computed for the imaginary part. >>> (I**(I+2)).as_real_imag() (-I**I, 0) Now it's clear to see that the imaginary part is zero. In is_comparable, the answer was being based on n if there was an i part that had no precision. It seems better to just return False in that case. This particular method should return T or F so if it's not clear that the imaginary part is 0 we'll say that the expression is not comparable.

If there is a rational coefficient then it might make sense to split the power up, making b**(c + r) = b**c*b**r

smichr · 2012-11-26T07:10:53Z

You might want to edit the commit message, changing 'comparions' to 'camparison' or 'comparisons'

sachin004 · 2012-11-26T20:02:36Z

Rebasing is showing many merge conflicts and I was not able to change the commit message.

smichr · 2012-11-27T04:53:02Z

Were you able to just cherry-pick my latest changes? If so commit that and we're done (just leave the typo in the commit message).

Otherwise, grab my branch and replace this one with that and, again, leave that typo commit alone:

git pull smichr
git checkout sachin1
git branch sachin1_bak
git checkout smichr/sachin1
git branch -D sachin1
git checkout -b sachin1
git push -f sachin004 sachin1

sachin004 · 2012-11-27T08:11:00Z

@smichr sachin@SACHIN-PC ~/sympy (sachin1)
$ git pull github smichr
Enter passphrase for key '/c/Users/sachin/.ssh/id_rsa':
fatal: Couldn't find remote ref smichr

sachin@SACHIN-PC ~/sympy (sachin1)
$ fatal: The remote end hung up unexpectedly

smichr · 2012-11-27T09:33:57Z

Should we just use the version I put up and then if you have additions do them as a PR to that?

#1674

sachin004 · 2012-11-27T09:43:18Z

That will be a good idea.

jrioux · 2012-11-27T17:32:04Z

SymPy Bot Summary: ✳️ Passed after merging sachin004/sachin1 (c870c5c) into master (6084fa8).
✳️ PyPy 2.0.0-beta-1; 2.7.3-final-42: pass
✳️ Python 2.7.2-final-0: pass
✳️ Python 3.2.1-final-0: pass
✳️ Sphinx 1.1.3: pass

Sachin1

smichr · 2012-11-28T03:37:43Z

OK, looks like the merge from my PR was successful. I don't have further comments and @flacjacket has already said this is ready, so...thanks, it's in.

added functionality to Q.real and Q.imaginary and fixed issue:2324

sachin004 · 2012-11-28T05:32:43Z

Thanks to everyone who helped me in completing this.

mrocklin reviewed Mar 21, 2012
View reviewed changes

flacjacket reviewed Mar 21, 2012
View reviewed changes

flacjacket reviewed Mar 23, 2012
View reviewed changes

smichr reviewed Oct 27, 2012
View reviewed changes

sachin004 and others added 5 commits November 24, 2012 20:12

Added test cases and removed unnecessary code

9bb61ed

3532: Pow.is_real fix for base = -1

b0edda5

Merge pull request #1 from smichr/sachin1

c870c5c

3532: Pow.is_real fix for base = -1

watch for Add in exponent for is_real

fb1b7f1

If there is a rational coefficient then it might make sense to split the power up, making b**(c + r) = b**c*b**r

smichr mentioned this pull request Nov 27, 2012

added functionality to Q.real and Q.imaginary and fixed issue:2324 #1674

Closed

Merge pull request #2 from smichr/sachin1

0c990c9

Sachin1

smichr added a commit that referenced this pull request Nov 28, 2012

Merge pull request #1156 from sachin004/sachin1

f4fc426

added functionality to Q.real and Q.imaginary and fixed issue:2324

smichr merged commit f4fc426 into sympy:master Nov 28, 2012

added functionality to Q.real and Q.imaginary and fixed issue:2324 #1156

added functionality to Q.real and Q.imaginary and fixed issue:2324 #1156

Conversation

sachin004 commented Mar 21, 2012

Choose a reason for hiding this comment

mrocklin commented Mar 21, 2012

sachin004 commented Mar 21, 2012

sachin004 commented Mar 21, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flacjacket commented Mar 21, 2012

Choose a reason for hiding this comment

flacjacket commented Mar 21, 2012

Choose a reason for hiding this comment

flacjacket commented Mar 22, 2012

mrocklin commented Mar 22, 2012

Choose a reason for hiding this comment

jrioux commented Oct 26, 2012

sachin004 commented Oct 26, 2012

flacjacket commented Oct 26, 2012

mrocklin commented Oct 26, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrioux commented Nov 19, 2012

jrioux commented Nov 24, 2012

sachin004 commented Nov 24, 2012

smichr commented Nov 26, 2012

sachin004 commented Nov 26, 2012

smichr commented Nov 27, 2012

sachin004 commented Nov 27, 2012

smichr commented Nov 27, 2012

sachin004 commented Nov 27, 2012

jrioux commented Nov 27, 2012

smichr commented Nov 28, 2012

sachin004 commented Nov 28, 2012