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

[WIP] Assumptions: make real -> finite #16603

Open
wants to merge 77 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@oscarbenjamin
Copy link
Contributor

commented Apr 8, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

This is first stab at making real imply finite and adding an is_extended_real assumption.

The main change so far is this:

diff --git a/sympy/core/assumptions.py b/sympy/core/assumptions.py
index ce4dd30f2e..7964bc7a89 100644
--- a/sympy/core/assumptions.py
+++ b/sympy/core/assumptions.py
@@ -165,25 +165,28 @@
     'integer        ->  rational',
     'rational       ->  real',
     'rational       ->  algebraic',
-    'algebraic      ->  complex',
+    'algebraic      ->  complex & finite',
     'real           ->  complex',
+    'real           ==  extended_real & finite',
+    'extended_real  ->  real | infinite',
     'real           ->  hermitian',
-    'imaginary      ->  complex',
+    'imaginary      ->  complex & finite',
     'imaginary      ->  antihermitian',
+    'extended_real  ->  commutative',
     'complex        ->  commutative',
 
     'odd            ==  integer & !even',
     'even           ==  integer & !odd',
 
-    'real           ==  negative | zero | positive',
-    'transcendental ==  complex & !algebraic',
+    'extended_real  ==  negative | zero | positive',
+    'transcendental ==  complex & !algebraic & finite',
 
     'negative       ==  nonpositive & nonzero',
     'positive       ==  nonnegative & nonzero',
     'zero           ==  nonnegative & nonpositive',
 
-    'nonpositive    ==  real & !positive',
-    'nonnegative    ==  real & !negative',
+    'nonpositive    ==  extended_real & !positive',
+    'nonnegative    ==  extended_real & !negative',
 
     'zero           ->  even & finite',
 
@@ -193,11 +196,11 @@
 
     'irrational     ==  real & !rational',
 
-    'imaginary      ->  !real',
+    'imaginary      ->  !extended_real',
 
     'infinite       ->  !finite',
-    'noninteger     ==  real & !integer',
-    'nonzero        ==  real & !zero',
+    'noninteger     ==  extended_real & !integer',
+    'nonzero        ==  extended_real & !zero',
 ])

Then there are many files where is_real has been replaced with is_extended_real and many tests that have been fixed.

Other comments

Release Notes

NO ENTRY

@sympy-bot

This comment has been minimized.

Copy link

commented Apr 8, 2019

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.

  • No release notes entry will be added for this pull request.

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.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed

This is first stab at making real imply finite and adding an is_extended_real assumption.

The main change so far is this:
```diff
diff --git a/sympy/core/assumptions.py b/sympy/core/assumptions.py
index ce4dd30f2e..7964bc7a89 100644
--- a/sympy/core/assumptions.py
+++ b/sympy/core/assumptions.py
@@ -165,25 +165,28 @@
     'integer        ->  rational',
     'rational       ->  real',
     'rational       ->  algebraic',
-    'algebraic      ->  complex',
+    'algebraic      ->  complex & finite',
     'real           ->  complex',
+    'real           ==  extended_real & finite',
+    'extended_real  ->  real | infinite',
     'real           ->  hermitian',
-    'imaginary      ->  complex',
+    'imaginary      ->  complex & finite',
     'imaginary      ->  antihermitian',
+    'extended_real  ->  commutative',
     'complex        ->  commutative',

     'odd            ==  integer & !even',
     'even           ==  integer & !odd',

-    'real           ==  negative | zero | positive',
-    'transcendental ==  complex & !algebraic',
+    'extended_real  ==  negative | zero | positive',
+    'transcendental ==  complex & !algebraic & finite',

     'negative       ==  nonpositive & nonzero',
     'positive       ==  nonnegative & nonzero',
     'zero           ==  nonnegative & nonpositive',

-    'nonpositive    ==  real & !positive',
-    'nonnegative    ==  real & !negative',
+    'nonpositive    ==  extended_real & !positive',
+    'nonnegative    ==  extended_real & !negative',

     'zero           ->  even & finite',

@@ -193,11 +196,11 @@

     'irrational     ==  real & !rational',

-    'imaginary      ->  !real',
+    'imaginary      ->  !extended_real',

     'infinite       ->  !finite',
-    'noninteger     ==  real & !integer',
-    'nonzero        ==  real & !zero',
+    'noninteger     ==  extended_real & !integer',
+    'nonzero        ==  extended_real & !zero',
 ])
 ```

Then there are many files where is_real has been replaced with is_extended_real and many tests that have been fixed.

#### Other comments



#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

There are many test failures here. I think I can fix them but there is a decision to be made so I am putting this up here first. This PR makes is_real imply is_finite and is_extended_real the current real. What should happen for is_negative, is_positive etc? They currently imply real but should they imply real or extended real?

If is_positive implies real then any number declared positive is automatically real and finite. On other hand this means that oo.is_positive would have to be False since oo is not real. That seems fine to me.

The alternative is that is_positive implies is_extended_real. Then a symbol declared with positive=True will not be known to be real since it could be infinite. This seems less desirable to me.

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

 'real           ->  complex',

I think that this should be preserved. oo and -oo cannot be considered complex in any reasonable way.

@oscarbenjamin oscarbenjamin force-pushed the oscarbenjamin:real_finite branch from 57964be to 666a7e1 Apr 10, 2019

@oscarbenjamin oscarbenjamin force-pushed the oscarbenjamin:real_finite branch from 46557c1 to ca69c02 Apr 11, 2019

@sylee957

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I see infinity is not contained in the Real set

from sympy import *
print(Contains(0, S.Reals))
print(Contains(oo, S.Reals))
print(Contains(-oo, S.Reals))

Would we also need an equivalent one in our set theorem module, like ExtendedReals?

@asmeurer

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Wow nice work. I think I tried doing this before and gave up, so good luck.

What do you think we should do with oo/-oo being positive/negative? Note that the following cannot all be simultaneously true:

  • "real" == finite real numbers
  • "positive" -> "real"
  • oo > 0
  • x > 0 iff x is positive

See #2538 (comment)

I see infinity is not contained in the Real set

Yes, infinities were not included in the sets. The motivation was that the sets should correspond to their usual mathematical definitions. It actually makes solveset simpler, because it doesn't have to worry about returning oo when it can be a "solution". If it comes up enough we can add an extended reals set, but you can always use Union(Reals, {-oo, oo}).

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

I am inclined to assuming "positive" -> "real". So x > 0 would mean "x is positive or oo".

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I am inclined to assuming "positive" -> "real". So x > 0 would mean "x is positive or oo".

I agree. The unintuitive implication is that oo.is_positive would be False but I think that's fine.

It was necessary in this PR to fix Pow.is_zero. On master we have:

In [1]: x = Symbol('x', positive=True)                                                                                                         

In [2]: (1/x).is_zero                                                                                                                          
Out[2]: False

Fixing that in this PR has lead to a proliferation of x = Symbol('x', positive=True, finite=True) in the tests.

Going down the positive->finite route I think that it would be useful to add is_extended_positive et al because otherwise there won't be anything equivalent to the current positive=True. Without extended_positive there would be no way to create a symbol with assumptions equivalent to current positive=True and no way to do an equivalent query on an object. Note that x.is_positive or x is S.Infinity etc are no equivalent.

Also adding extended_positive would make the implementation easier for now since you can just replace _eval_is_positive and is_positive with _eval_is_extended_positive and is_extended_positive in SymPy's internals. That is what I've done here for _eval_is_real and I mostly just needed to fix a few other _eval_is methods (e.g. finite) to make that work. That might not be the best way to implement things longer term but it certainly helps just for getting this change over the line.

Since this PR is close to passing the tests I propose to open a new PR based off of here in which positive->finite and there are new assumptions such as is_extended_positive.

Can I get some +1/-1 style comments on that idea? These are time consuming changes to implement so I'd like to know as soon as possible if the idea isn't liked!

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Also in many places I've need to change Symbol('z', real=False) to Symbol('z', extended_real=False). I wonder it would be worthwhile to have a nonreal assumption like:

    nonreal == complex & !real
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I've XFAILed the two remaining non-slow failing tests to see how this fares with the rest of the test suite.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

These are the two test failures that I XFAILed:

$ pytest -m 'not slow' --lf
============================================================= test session starts =============================================================
platform darwin -- Python 3.7.1, pytest-4.1.2.dev19+g57bf9d67, py-1.7.0, pluggy-0.8.0
rootdir: /Users/enojb/current/sympy/sympy, inifile: pytest.ini
plugins: xdist-1.26.0, instafail-0.4.1, forked-0.2, cov-2.6.1, doctestplus-0.3.0.dev0
collected 8805 items / 8803 deselected                                                                                                        
run-last-failure: rerun previous 9 failures

sympy/series/tests/test_limits.py F                                                                                                     [ 50%]
sympy/series/tests/test_order.py F                                                                                                      [100%]

================================================================== FAILURES ===================================================================
______________________________________________________________ test_issue_12564 _______________________________________________________________

    def test_issue_12564():
>       assert limit(x**2 + x*sin(x) + cos(x), x, -oo) == oo
E       assert AccumBounds(-oo, oo) == oo
E        +  where AccumBounds(-oo, oo) = limit((((x ** 2) + (x * sin(x))) + cos(x)), x, -oo)
E        +    where sin(x) = sin(x)
E        +    and   cos(x) = cos(x)

sympy/series/tests/test_limits.py:507: AssertionError
_______________________________________________________________ test_contains_4 _______________________________________________________________

    def test_contains_4():
>       assert Order(sin(1/x**2)).contains(Order(cos(1/x**2))) is None
E       assert True is None
E        +  where True = <bound method Order.contains of O(sin(x**(-2)))>(O(cos(x**(-2))))
E        +    where <bound method Order.contains of O(sin(x**(-2)))> = O(sin(x**(-2))).contains
E        +      where O(sin(x**(-2))) = Order(sin(x**(-2)))
E        +        where sin(x**(-2)) = sin((1 / (x ** 2)))
E        +    and   O(cos(x**(-2))) = Order(cos(x**(-2)))
E        +      where cos(x**(-2)) = cos((1 / (x ** 2)))

sympy/series/tests/test_order.py:136: AssertionError
                                                               DO *NOT* COMMIT!                                                               
================================================= 2 failed, 8803 deselected in 44.23 seconds ==================================================

They are both in parts of the codebase that I am not familiar with so it's not easy for me to debug them. Note that there are a number of other small fixes/changes to various _eval_is methods so it's not as simple as looking for is_real or is_finite in the relevant code.

@codecov

This comment has been minimized.

Copy link

commented Apr 11, 2019

Codecov Report

Merging #16603 into master will decrease coverage by 0.045%.
The diff coverage is 92.198%.

@@              Coverage Diff              @@
##            master    #16603       +/-   ##
=============================================
- Coverage   73.847%   73.802%   -0.046%     
=============================================
  Files          619       619               
  Lines       159329    159268       -61     
  Branches     37388     37383        -5     
=============================================
- Hits        117660    117543      -117     
- Misses       36241     36285       +44     
- Partials      5428      5440       +12
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I've fixed some slow tests, commented out one assert in test_meijerint and XFAILED test_W23 in test_wester.

I think the tests should pass now.

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

The limits test fails because AccumBounds(-oo, oo).is_finite has changed from None to False. It is tested on this line.

if l.has(S.Infinity) and l.is_finite is None:

I am not sure which should be the proper value, None or False. Maybe the test could be fixed by changing the condition to not l.is_finite (but that might break something else).

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

AccumBounds(-oo, oo).is_finite should probably be None although the endpoints -oo and oo are understood to be included.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

It would be good to note how often positive in the codebase should naturally include oo (same with real).

Regarding extended_positive, if it's needed a lot, that could be a sign that positive ought to include oo. Also ideally we would be able to use the new assumptions ask(x > 0), which I think should include oo regardless whether or not "positive" does.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

In other words, the tests pass, but did you do an audit of the uses of real in the codebase to make sure they shouldn't be changed to extended_real? Based on the diff here I'm guessing you did.

@asmeurer

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Can I get some +1/-1 style comments on that idea? These are time consuming changes to implement so I'd like to know as soon as possible if the idea isn't liked!

I'm not clear exactly what part of the plan you are unsure about, but it sounds fine to me.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

AccumBounds(-oo, oo).is_finite should probably be None although the endpoints -oo and oo are understood to be included.

Thanks for this. I added _eval_is_real here:
https://github.com/sympy/sympy/pull/16603/files#diff-a5d2575656fef33ceed5fc4cd005e3c0R891
Since that returns False if the end points are infinite that will imply finite=False.

I'll try fixing that to return None if either end point is infinite.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I'll try fixing that to return None if either end point is infinite.

I've tried that and it seems to work so I've removed that XFAIL.

@jksuom

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

The order test seems to pass as well.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

The order test seems to pass as well.

Thanks again!

That means that there is one outstanding XFAIL:

$ pytest sympy/utilities/tests/test_wester.py -k test_W23 --runxfail 
============================================================= test session starts =============================================================
platform darwin -- Python 3.7.1, pytest-4.1.2.dev19+g57bf9d67, py-1.7.0, pluggy-0.8.0
architecture: 64-bit
cache:        yes
ground types: gmpy 1.17

rootdir: /Users/enojb/current/sympy/sympy, inifile: pytest.ini
plugins: xdist-1.26.0, instafail-0.4.1, forked-0.2, cov-2.6.1, doctestplus-0.3.0.dev0
collected 397 items / 395 deselected                                                                                                          

sympy/utilities/tests/test_wester.py F.                                                                                                 [100%]

================================================================== FAILURES ===================================================================
__________________________________________________________________ test_W23 ___________________________________________________________________

    @XFAIL
    @slow
    def test_W23():
        a, b = symbols('a b', real=True, positive=True)
        r1 = integrate(integrate(x/(x**2 + y**2), (x, a, b)), (y, -oo, oo))
>       assert r1.collect(pi) == pi*(-a + b)
E       assert 0 == (pi * (-a + b))
E        +  where 0 = <bound method Expr.collect of 0>(pi)
E        +    where <bound method Expr.collect of 0> = 0.collect

sympy/utilities/tests/test_wester.py:2548: AssertionError
                                                               DO *NOT* COMMIT!                                                               
============================================= 1 failed, 1 passed, 395 deselected in 12.75 seconds =============================================
@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

In other words, the tests pass, but did you do an audit of the uses of real in the codebase to make sure they shouldn't be changed to extended_real? Based on the diff here I'm guessing you did.

I didn't just change real to extended_real en masse. I first changed the assumptions then ran the tests and then investigated the failures to see if changes were needed.

I couldn't possibly go through all of the logic of all of the _eval_is_real methods though. For many of these the fix is just _eval_is_real -> _eval_is_extended_real and is_real -> is_extended_real. Since that doesn't actually change the logic those changes should be valid. They still have effects though since is_real won't always give the same answer (but that's the whole point of the PR).

Defining _eval_is_extended_real rather than _eval_is_real is often needed because with the assumptions as they are in this PR extended_real can only be inferred from negative, zero or positive so if we don't know whether something is positive or negative none of the other assumptions can imply extended_real. On the other hand is_real can be inferred from is_extended_real and is_finite. In some cases I needed to add/improve _eval_is_finite to keep is_real working. In some cases I could improve other parts of the assumption handling code e.g. Pow.is_zero etc to get things working.

In many cases I needed to change the assumptions in the test. There are currently many tests that shouldn't pass if infinite values are properly considered. This happens particularly in stats where e.g. we can't calculated the moments of a distribution if the mean, variance etc are infinite.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

Defining _eval_is_extended_real rather than _eval_is_real is often needed because with the assumptions as they are in this PR extended_real can only be inferred from negative, zero or positive so if we don't know whether something is positive or negative none of the other assumptions can imply extended_real.

What I said there isn't right: we obviously have real->extended_real. The issue is with the inverted condition: not real does not imply not extended_real. So "not extended_real" cannot be inferred from anything else.

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

So "not extended_real" cannot be inferred from anything else.

Again that's not right! We have that "finite and not real" implies "not extended_real".

So I guess it isn't usually necessary to define _eval_is_extended_real. It's just the simplest way to implement this change right now.

@oscarbenjamin oscarbenjamin force-pushed the oscarbenjamin:real_finite branch from f658f83 to 3e058c6 Apr 22, 2019

@oscarbenjamin

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I've rebased to resolve conflicts with #16592

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.