-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Make positive -> finite with new facts like is_extended_positive #16666
Conversation
✅ Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5. Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
I've just pushed a commit to fix the failing test. The issue was due to Expr not having _eval_is_positive. I think that maybe the code for Expr._eval_is_extended_positive/negative should be factored out. If I understand correctly the code currently in _eval_is_extended_positive doesn't handle infinities and should really be in a _eval_is_positive method. Expr needs to define _eval_is_extended_positive so that it can be called from Add._eval_is_extended_positive in
I'm not sure if that can just be changed back to calling _eval_is_positive because I don't know if super would always find Expr as the superclass and other classes might implement _eval_is_extended_positive differently taking account of infinities as well. The commit that I just pushed includes (in Expr): def _eval_is_extended_positive(self):
if self.is_extended_real is False:
return False
if Expr._eval_is_positive(self):
return True |
This PR currently has 3 XFAILS. There are two for limits: $ pytest sympy/utilities/tests/test_wester.py --runxfail -k test_T6
============================================================= test session starts =============================================================
...
================================================================== FAILURES ===================================================================
___________________________________________________________________ test_T6 ___________________________________________________________________
@XFAIL
def test_T6():
> assert limit(1/n * factorial(n)**(1/n), n, oo) == exp(-1)
E assert 0 == exp(-1)
E + where 0 = limit(((1 / n) * (factorial(n) ** (1 / n))), n, oo)
E + where factorial(n) = factorial(n)
E + and exp(-1) = exp(-1)
sympy/utilities/tests/test_wester.py:2093: AssertionError
DO *NOT* COMMIT!
================================================== 1 failed, 396 deselected in 1.03 seconds =================================================== $ pytest sympy/series/tests/test_limits.py --runxfail -k test_factorial
============================================================= test session starts =============================================================
================================================================== FAILURES ===================================================================
_______________________________________________________________ test_factorial ________________________________________________________________
@XFAIL
def test_factorial():
from sympy import factorial, E
f = factorial(x)
assert limit(f, x, oo) == oo
> assert limit(x/f, x, oo) == 0
E AssertionError: assert Limit(x/factorial(x), x, oo, dir='-') == 0
E + where Limit(x/factorial(x), x, oo, dir='-') = limit((x / factorial(x)), x, oo)
sympy/series/tests/test_limits.py:412: AssertionError
DO *NOT* COMMIT!
=================================================== 1 failed, 61 deselected in 0.70 seconds =================================================== The third XFAIL is to do with the new assumptions calling the old assumptions. I think that's because I haven't updated the new assumptions yet but I'm not sure: $ pytest sympy/assumptions/tests/test_sathandlers.py --runxfail -k test_CheckOldAssump
============================================================= test session starts =============================================================
================================================================== FAILURES ===================================================================
_____________________________________________________________ test_CheckOldAssump _____________________________________________________________
@XFAIL
def test_CheckOldAssump():
# TODO: Make these tests more complete
class Test1(Expr):
def _eval_is_positive(self):
return True
def _eval_is_negative(self):
return False
class Test2(Expr):
def _eval_is_finite(self):
return True
def _eval_is_positive(self):
return True
def _eval_is_negative(self):
return False
t1 = Test1()
t2 = Test2()
# We can't say if it's positive or negative in the old assumptions without
# bounded. Remember, True means "no new knowledge", and
# Q.positive(t2) means "t2 is positive."
> assert CheckOldAssump(Q.positive(t1)) == True
E assert Q.positive(Test1()) == True
E + where Q.positive(Test1()) = CheckOldAssump(Q.positive(Test1()))
E + where Q.positive(Test1()) = Q.positive(Test1())
E + where Q.positive = <sympy.assumptions.ask.AssumptionKeys object at 0x108696320>.positive
sympy/assumptions/tests/test_sathandlers.py:94: AssertionError
DO *NOT* COMMIT!
=================================================== 1 failed, 5 deselected in 0.18 seconds ==================================================== |
sympy/concrete/products.py
Outdated
pi/2 | ||
|
||
#>>> limit(W2e, n, oo) | ||
#pi/2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have commented out this doctest because it failed:
092 >>> from sympy import limit
093 >>> W2 = Product(2*i/(2*i-1)*2*i/(2*i+1), (i, 1, n))
094 >>> W2
095 Product(4*i**2/((2*i - 1)*(2*i + 1)), (i, 1, n))
096 >>> W2e = W2.doit()
097 >>> W2e
098 2**(-2*n)*4**n*factorial(n)**2/(RisingFactorial(1/2, n)*RisingFactorial(3/2, n))
099
100 >>> limit(W2e, n, oo)
Expected:
pi/2
Got:
Limit(2**(-2*n)*4**n*factorial(n)**2/(RisingFactorial(1/2, n)*RisingFactorial(3/2, n)), n, oo, dir='-')
sympy/concrete/products.py:100: DocTestFailure
Codecov Report
@@ Coverage Diff @@
## master #16666 +/- ##
=============================================
- Coverage 73.906% 73.897% -0.009%
=============================================
Files 619 619
Lines 160001 160101 +100
Branches 37554 37578 +24
=============================================
+ Hits 118251 118311 +60
- Misses 36269 36308 +39
- Partials 5481 5482 +1 |
The tests have passed (with 3 XFAILED and one doctest commented out) but have taken longer than usual to run. I can reproduce a slowdown with:
That takes 24 seconds with this PR and 14 seconds on master. |
There is an intermittent test failure that I can reproduce on Python 3.7 with: $ PYTHONHASHSEED=2937395614 bin/test sympy/core/tests/test_assumptions.py -k test_Mul_is_prime_composite --seed=73309503
============================================================= test process starts =============================================================
executable: /Users/enojb/current/sympy/venv/bin/python (3.7.1-final-0) [CPython]
architecture: 64-bit
cache: yes
ground types: gmpy 1.17
numpy: 1.16.1
random seed: 73309503
hash randomization: on (PYTHONHASHSEED=2937395614)
sympy/core/tests/test_assumptions.py[1] F [FAIL]
_______________________________________________________________________________________________________________________________________________
______________________________________ sympy/core/tests/test_assumptions.py:test_Mul_is_prime_composite _______________________________________
Traceback (most recent call last):
File "/Users/enojb/current/sympy/sympy/sympy/core/tests/test_assumptions.py", line 834, in test_Mul_is_prime_composite
assert ( (x+1)*(y+1) ).is_prime is False
AssertionError
============================================= tests finished: 0 passed, 1 failed, in 0.28 seconds =============================================
DO *NOT* COMMIT! |
I think I've fixed the performance problem. I've also fixed the 2 XFAILed tests and one commented test that were all to do with limits. That leaves one XFAILed test to do with the new assumptions: $ pytest sympy/assumptions/tests/test_sathandlers.py --runxfail -k test_CheckOldAssump I think that's just because I should update the new assumptions now. |
It looks like the new assumptions already define positive as finite: sympy/sympy/assumptions/sathandlers.py Line 186 in 3335542
So I think the only problem there is that the failing test is no longer valid under the changes in this PR: $ pytest sympy/assumptions/tests/test_sathandlers.py --runxfail -k test_CheckOldAssump
============================================================= test session starts =============================================================
================================================================== FAILURES ===================================================================
_____________________________________________________________ test_CheckOldAssump _____________________________________________________________
@XFAIL
def test_CheckOldAssump():
# TODO: Make these tests more complete
class Test1(Expr):
def _eval_is_positive(self):
return True
def _eval_is_negative(self):
return False
class Test2(Expr):
def _eval_is_finite(self):
return True
def _eval_is_positive(self):
return True
def _eval_is_negative(self):
return False
t1 = Test1()
t2 = Test2()
# We can't say if it's positive or negative in the old assumptions without
# bounded. Remember, True means "no new knowledge", and
# Q.positive(t2) means "t2 is positive."
> assert CheckOldAssump(Q.positive(t1)) == True
E assert Q.positive(Test1()) == True
E + where Q.positive(Test1()) = CheckOldAssump(Q.positive(Test1()))
E + where Q.positive(Test1()) = Q.positive(Test1())
E + where Q.positive = <sympy.assumptions.ask.AssumptionKeys object at 0x108696320>.positive
sympy/assumptions/tests/test_sathandlers.py:94: AssertionError |
I've fixed that test with positive -> extended_positive. That means there are no failing tests with this PR now. Todo:
Reviews/suggestions welcome! |
cd32f56
to
44a93f6
Compare
Thanks you very much for taking the time to review this @ShubhamKJha. I think I've responded to all of your comments now but please let me know if I've missed something. I think I've done everything needed in this PR now apart from squashing the commits. Is there any useful way to format them? I don't think that the changes can be separated into independently revertible parts but I can split it into maybe one commit for the changes to the core, another for functions, and several for different parts of the tests. |
I'm not sure that keeping all the commit separate will help in tracking down any future bugs, but one will be able to use bisection to still tell if it is before or after this batch that something broke so I don't think there is a need to squash them. This needs a release note telling what has changed, i.e. what worked before vs what works now. Is this backwards incompatible in assumptions in any way? |
I've added a release note. This is backwards incompatible. I find it hard to gauge how many problems users will have with it. I had to change a lot of things from e.g. is_real to is_extended_real but I suspect that this is more a problem in the core than it would be for users. Perhaps a more likely problem for users and one that came up a few times in this PR is when assumptions are declared negatively e.g.
Where as with this PR:
I suspect that most users (and contributors?) who write negative=False don't realise what it actually means and should really use nonnegative=True or include real=True. I've mentioned this in the release notes. |
I have no objections but let's wait for a sign-off from @ShubhamKJha. |
What is the status of the new assumptions with this? |
I don't understand what the situation with the new assumptions is. There is Q.real and also Q.extended_real but Q.real does not imply finite and Q.real implies Q.extended_real:
Also this code suggests that the new assumptions are different somehow in this respect but I don't see how in terms of output: sympy/sympy/assumptions/sathandlers.py Line 186 in 3335542
|
Whoops! Q.real implies Q.extended_real is not surprising. That's what this PR does with the old assumptions. In the new assumptions Q.extended_real does not imply Q.real:
However Q.real does not imply finite:
so I don't know what that means. |
We can clean up the new assumptions in a new PR. The two should ultimately be consistent with one another. I just wanted to see if that was already the case or not. |
That is correct. |
Thanks @oscarbenjamin this PR looks good. There are certainly two issues that popped up:
These can be taken care of in another PR. |
I think it is time to merge this. |
I think you have the right behavior here |
Thanks everyone for reviewing and for merging. There are a number of followup tasks that I'll list here:
For 3. I don't have time for a while now but I can get to it at some point. If someone else wants to do it sooner then go ahead! I didn't update the docs yet because I viewed this as a step towards making complex imply finite. The docs will need updating before the next release in any case though. Also it looks like the core
|
It's the right behaviour for the wrong reason because after this PR we still have:
This is due to this code in Lines 365 to 369 in b9604c5
I have to say I don't understand exactly why that code is written the way it is but it doesn't handle the negative assumption case correctly. |
References to other Issues or PRs
Based on #16603
Alternative to #16655
Brief description of what is fixed or changed
This is an attempt to build on #16603 which makes is_real imply is_finite. The idea is to also make positive, nonpositive etc also imply finite so that positive implies real. In #16655 I tried to do this without introducing new facts and hit a dead end. This attempt introduces new facts such as is_extended_positive which allow for positive testing with oo and -oo included. The assumption rules used here are:
Note that many of these rules are redundant. I initially had a more economical set of implications but deduce_all_facts was unable to make all possible inferences so I added additional rules to help it.
Other comments
Release Notes
Symbol('x', real=True)
in version 1.4 is nowSymbol('x', extended_real=True)
. The equivalent ofSymbol('x', negative=False)
is nowSymbol('x', extended_negative=False)
although it is usually better to useSymbol('x', nonnegative=True)
(which implies both real=True and finite=True as well). Code that previously checkedif x.is_positive
should now be written asif x.is_extended_positive
if it is intended that infinities should be allowed.