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] Make complex->finite in the codebase. #16978

Closed
wants to merge 15 commits into from

Conversation

@ShubhamKJha
Copy link
Member

@ShubhamKJha ShubhamKJha commented Jun 6, 2019

References to other Issues or PRs

Brief description of what is fixed or changed

[Do not merge this PR]
This PR attempts to make is_complex imply is_finite. The assumption rules used are:

_assume_rules = FactRules([

    'integer        ->  rational',
    'rational       ->  real',
    'rational       ->  algebraic',
    'algebraic      ->  complex',
    'transcendental ==  complex & !algebraic',
    'real           ->  hermitian',
    'imaginary      ->  complex',
    'imaginary      ->  antihermitian',
    'extended_real  ->  commutative',
    'complex        ->  commutative',
    'complex        ->  finite',

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

    'real           ->  complex',
    'extended_real  ->  real | infinite',
    'real           ==  extended_real & finite',

    'extended_real        ==  extended_negative | zero | extended_positive',
    'extended_negative    ==  extended_nonpositive & extended_nonzero',
    'extended_positive    ==  extended_nonnegative & extended_nonzero',

    'extended_nonpositive ==  extended_real & !extended_positive',
    'extended_nonnegative ==  extended_real & !extended_negative',

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

    'nonpositive    ==  real & !positive',
    'nonnegative    ==  real & !negative',

    'positive       ==  extended_positive & finite',
    'negative       ==  extended_negative & finite',
    'nonpositive    ==  extended_nonpositive & finite',
    'nonnegative    ==  extended_nonnegative & finite',
    'nonzero        ==  extended_nonzero & finite',

    'zero           ->  even & finite',
    'zero           ==  extended_nonnegative & extended_nonpositive',
    'zero           ==  nonnegative & nonpositive',
    'nonzero        ->  real',

    'prime          ->  integer & positive',
    'composite      ->  integer & positive & !prime',
    '!composite     ->  !positive | !even | prime',

    'irrational     ==  real & !rational',

    'imaginary      ->  !extended_real',

    'infinite       ->  !finite',
    'noninteger     ==  extended_real & !integer',
    'extended_nonzero == extended_real & !zero',
])

Other comments

This breaks zoo.is_complex throughout the codebase. I have tried to do this without including any extra facts (i.e. extended_complex for entities like zoo)

Release Notes

  • core
    • The assumptions system is changed so now only finite values will be considered complex. With this zoo is no longer complex in this sense. And hence, zoo.is_complex now returns False. Also, any symbol with assumption complex = True would become finite and with finite = False will become complex = False by default.
    • A number of instances of _eval_is_complex is changed to better accommodate the finite nature of them.
@sympy-bot
Copy link

@sympy-bot sympy-bot commented Jun 6, 2019

Hi, I am the SymPy bot (v149). 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:

  • core
    • The assumptions system is changed so now only finite values will be considered complex. With this zoo is no longer complex in this sense. And hence, zoo.is_complex now returns False. Also, any symbol with assumption complex = True would become finite and with finite = False will become complex = False by default. (#16978 by @ShubhamKJha)

    • A number of instances of _eval_is_complex is changed to better accommodate the finite nature of them. (#16978 by @ShubhamKJha)

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.

<!-- 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
**[Do not merge this PR]**
This PR attempts to make `is_complex` imply `is_finite`. The assumption rules used are:
```py
_assume_rules = FactRules([

    'integer        ->  rational',
    'rational       ->  real',
    'rational       ->  algebraic',
    'algebraic      ->  complex',
    'transcendental ==  complex & !algebraic',
    'real           ->  hermitian',
    'imaginary      ->  complex',
    'imaginary      ->  antihermitian',
    'extended_real  ->  commutative',
    'complex        ->  commutative',
    'complex        ->  finite',

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

    'real           ->  complex',
    'extended_real  ->  real | infinite',
    'real           ==  extended_real & finite',

    'extended_real        ==  extended_negative | zero | extended_positive',
    'extended_negative    ==  extended_nonpositive & extended_nonzero',
    'extended_positive    ==  extended_nonnegative & extended_nonzero',

    'extended_nonpositive ==  extended_real & !extended_positive',
    'extended_nonnegative ==  extended_real & !extended_negative',

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

    'nonpositive    ==  real & !positive',
    'nonnegative    ==  real & !negative',

    'positive       ==  extended_positive & finite',
    'negative       ==  extended_negative & finite',
    'nonpositive    ==  extended_nonpositive & finite',
    'nonnegative    ==  extended_nonnegative & finite',
    'nonzero        ==  extended_nonzero & finite',

    'zero           ->  even & finite',
    'zero           ==  extended_nonnegative & extended_nonpositive',
    'zero           ==  nonnegative & nonpositive',
    'nonzero        ->  real',

    'prime          ->  integer & positive',
    'composite      ->  integer & positive & !prime',
    '!composite     ->  !positive | !even | prime',

    'irrational     ==  real & !rational',

    'imaginary      ->  !extended_real',

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

#### Other comments
This breaks `zoo.is_complex` throughout the codebase. I have tried to do this without including any extra facts (i.e. `extended_complex` for entities like `zoo`)

#### 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 -->
* core
  * The assumptions system is changed so now only finite values will be considered complex. With this `zoo` is no longer complex in this sense. And hence, `zoo.is_complex` now returns `False`. Also, any symbol with assumption `complex = True` would become `finite ` and with `finite = False` will become `complex = False` by default.
  * A number of instances of `_eval_is_complex` is changed to better accommodate the finite nature of them. 
<!-- END RELEASE NOTES -->

sympy/algebras/quaternion.py Outdated Show resolved Hide resolved
sympy/core/numbers.py Outdated Show resolved Hide resolved
@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Jun 7, 2019

This generally looks good although I left comments

There are lots of places where you've changed e.g. real->extended_real or positive->extended_positive. Are those needed because of the changes here or is that just unfinished business from #16666?

It looks like there are a number of places where you are testing if x.is_complex or x.is_infinite. Potentially those would benefit from being x.is_extended_complex. How many places are there in the code where x.is_complex is used and how many need to also check for x.is_finite? I count 40 occurrences of is_complex:

$ git grep is_complex | grep -v test_ | wc -l
      40

@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Jun 7, 2019

There are lots of places where you've changed e.g. real->extended_real or positive->extended_positive. Are those needed because of the changes here or is that just unfinished business from #16666?

I tried not to revert any from #16666, most of them appeared to be left unchanged during #16666.

Apart from the occurrences of is_complex I also checked the occurrences of complex and changed when seemed appropriate.
The present notion of complex is that of extended_complex, therefore, I changed the instances of is_complex to is_complex or is_infinite to better account for oo, -oo, zoo.

@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Jun 7, 2019

Thanks @oscarbenjamin for this review, I will go through the diff for the instances where is_complex is actually asking for finite complex.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Jun 21, 2019

This branch has merge conflicts now

@smichr smichr changed the title [WIP] Make comple->finite in the codebase. [WIP] Make complex->finite in the codebase. Jun 21, 2019
sympy/core/assumptions.py Outdated Show resolved Hide resolved
@@ -332,7 +332,7 @@ def __ge__(self, other):
except SympifyError:
raise TypeError("Invalid comparison %s >= %s" % (self, other))
for me in (self, other):
if me.is_complex and me.is_extended_real is False:
if (me.is_complex or me.is_infinite) and me.is_extended_real is False:
Copy link
Contributor

@oscarbenjamin oscarbenjamin Jun 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this replacement will not be equivalent since if x is complex then (1/x) will be complex or infinite but (1/x).is_complex and (1/x).is_infinite will both return None. It isn't possible to make something equivalent without having extended_complex.

Copy link
Contributor

@oscarbenjamin oscarbenjamin Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the is_complex check here at all? Shouldn't we just raise if extended_real is False?

Copy link
Contributor

@oscarbenjamin oscarbenjamin Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Same goes for the other comparison methods below.)

@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Aug 22, 2019

ping @oscarbenjamin @asmeurer @jksuom. Are we still in support of this? I think it can be merged.

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 22, 2019

Let's decide either way on this before the next release.

@asmeurer asmeurer added this to the SymPy 1.5 milestone Aug 22, 2019
@jksuom
Copy link
Member

@jksuom jksuom commented Aug 22, 2019

Having complex => finite looks good to me. I think that it should be accepted if possible without too many issues.

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 22, 2019

There's still a comment on the diff about quaternions not accepting infinite values. Were all the instances where is_infinite was added checked to see if infinite really makes sense there? I know it's the same behavior as before, but it would be really confusing to have that explicitly in the code where it doesn't really belong.

@codecov
Copy link

@codecov codecov bot commented Aug 23, 2019

Codecov Report

Merging #16978 into master will decrease coverage by 0.018%.
The diff coverage is 94.444%.

@@              Coverage Diff              @@
##            master    #16978       +/-   ##
=============================================
- Coverage   74.716%   74.698%   -0.019%     
=============================================
  Files          632       633        +1     
  Lines       164357    164443       +86     
  Branches     38579     38606       +27     
=============================================
+ Hits        122802    122836       +34     
- Misses       36170     36220       +50     
- Partials      5385      5387        +2

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 23, 2019

I will go through the diff soon but in general I think that this should be merged for the next release. The real->Finite changes are since the last release so merging this as well means they both change simultaneously.

sympy/core/tests/test_arit.py Outdated Show resolved Hide resolved
arg = self.args[0]

if arg.is_real and (arg / pi - S.Half).is_integer is False:
return True
Copy link
Contributor

@oscarbenjamin oscarbenjamin Aug 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we had extended_complex then methods like this wouldn't be needed. We could just say that extended_complex is True and let _eval_is_finite imply complex.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 23, 2019

I guess the only question here is whether we want extended_complex. I think this diff would look very different if we went down that road.

@jksuom
Copy link
Member

@jksuom jksuom commented Aug 23, 2019

I think that symbols are expected to be extended complex by default (though that may not have been explicitly told anywhere). If that is understood, then there will probably be no need for extended_complex assumption.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 23, 2019

I think that symbols are expected to be extended complex by default (though that may not have been explicitly told anywhere). If that is understood, then there will probably be no need for extended_complex assumption.

The question is how do you test for it. The diff here has a load of if x.is_complex or x.is_finite but that won't work for say 1/x where x is complex since both conditions will give None. Likewise what is the test that differentiates between a vanilla symbol with implicit extended complex assumptions and, say a MatrixSymbol or a quaternion or something?

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 23, 2019

By default they are commutative, but I guess you can basically see commutative as equal to extended complex. I think that's not actually strictly true because you might have some other commutative objects that aren't extended complex (like some elements of some commutative algebra perhaps).

I think in practice it means we have to check only for negative conditions, so instead of

if x.is_complex is True: good

use

if x.is_complex is False: bad

That's similar to how we handle functions with implicit domains, for instance, if a function only makes sense for integer inputs, we fail for is_integer=False but not for is_integer=None.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 23, 2019

I think we should merge this. We can add extended complex later if needed. Let's get this in to the next release and let users see it.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 24, 2019

Another example:

In [11]: f = Lambda(x, x**2)                                                                                                      

In [12]: f.is_complex == None                                                                                                     
Out[12]: True

In [13]: exp(f)                                                                                                                   
Out[13]: 
      2
 x ↦ x 
ℯ  

What test should exp use to check that its argument is a legit "number"?

Here's another:

In [14]: exp(sin)                                                                                                                 
Out[14]: 
 sin
ℯ   

In [15]: sin.is_complex                                                                                                           
Out[15]: <property at 0x104f61e08>

In [16]: bool(sin.is_complex)                                                                                                     
Out[16]: True

In [19]: bool(sin.is_commutative)                                                                                                 
Out[19]: True

@jksuom
Copy link
Member

@jksuom jksuom commented Aug 24, 2019

We should be able to set is_commutative and is_complex false for functions. Maybe by defining _explicit_class_assumptions.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Aug 24, 2019

I still think there's a need to have a property that corresponds to what would be expected as input by most mathematical functions which is basically complex, oo, -oo and zoo and quantities derived from them like 1+I*oo. The obvious property name would be something like is_number but we already have that and it means something different.

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Sep 9, 2019

This needs reviving. We shouldn't forget this before the next release. There are some test failures but they probably aren't hard to fix.

The discussion above seems to have gotten bogged down in the question of whether we need extended_complex but I think we can get this merged and worry about whether we need that later.

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Sep 10, 2019

Yes, this is blocking a release.

@oscarbenjamin oscarbenjamin mentioned this pull request Oct 4, 2019
@ShubhamKJha ShubhamKJha deleted the complex_finite branch Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants