Better `eval` and `is_zero` checking for a number of functions. #17510

oscargus · 2019-08-25T13:37:26Z

…appelf1 and hyper

References to other Issues or PRs

Brief description of what is fixed or changed

Many functions now use is_zero in eval (typically towards the end of eval for performance reasons). This will lead to smaller expressions in general.

Many functions now have a dedicated _eval_is_zero function.

Also, added support for evaluate-keywords to hyper and appellf1. hyper never evaluates, but I think it makes sense to support it anyway, if nothing else for the is_zero and is_rational-support.

Other comments

Release Notes

functions
- hyper and appellf1 supports evaluate keyword.
- Many functions now use is_zero to evaluate which leads to smaller expressions.
- Better is_zero check for many functions.
- beta evaluates expressions if one of the arguments is 1.

sympy-bot · 2019-08-25T13:37:27Z

✅

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:

functions
- hyper and appellf1 supports evaluate keyword. (#17510 by @oscarbenjamin and @oscargus)
- Many functions now use is_zero to evaluate which leads to smaller expressions. (#17510 by @oscarbenjamin and @oscargus)
- Better is_zero check for many functions. (#17510 by @oscarbenjamin and @oscargus)
- beta evaluates expressions if one of the arguments is 1. (#17510 by @oscarbenjamin and @oscargus)

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.

…appelf1 and hyper

<!-- 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. -->
Fixes #17511

#### Brief description of what is fixed or changed
Many functions now use `is_zero` in `eval` (typically towards the end of `eval` for performance reasons). This will lead to smaller expressions in general.

Many functions now have a dedicated `_eval_is_zero` function.

Also, added support for `evaluate`-keywords to `hyper` and `appellf1`. `hyper` never evaluates, but I think it makes sense to support it anyway, if nothing else for the `is_zero` and `is_rational`-support.

#### 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 -->
* functions
   * `hyper` and `appellf1` supports `evaluate` keyword.       
   * Many functions now use `is_zero` to evaluate which leads to smaller expressions.
   * Better `is_zero` check for many functions.
   * `beta` evaluates expressions if one of the arguments is `1`.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

oscarbenjamin · 2019-08-25T14:23:08Z

sympy/core/function.py

+    def _eval_is_zero(self):
+        f = self.func(*self.args, evaluate=True)
+        if f.func != self.func:
+            return f.is_zero


I'm not sure about this. I have generally approached this from the perspective that if the user doesn't want to evaluate then the assumptions system shouldn't do the evaluation. We should generally assume that the user had a good reason for not evaluating.

The whole idea is really that I think all checks should be done as .is_zero rather than == 0. In the longer run, the functions can catch up to return better results.

Note that Add(1, -1, evaluate=False).is_zero is currently True.

Part of the motivation comes from #17454 and #17482 where the polynomial has a form of x**2 + x*expr but in a later stage it turned out that expr was 0. Hence, checking for == 0 or is S.Zero is not really viable, but with a good is_zero things would be easier.

Also, consider Sum(0, (x, M, N)). is_zero, sin(Symbol('x', zero=True)), #17414, and the Zen of SymPy: "Unevaluated is better than evaluated.". If one would like to know if the expression has evaluated to 0 or S.Zero check against that. If one would like to know if the expression (for any reason) will evaluate or has evaluated to zero, check is_zero. As the assumptions can figure out things that the numerical evaluation cannot, it makes sense to primarily use that when possible in most code.

Somehow, it is a question if the numerical comparisons should fallback to the assumptions or the assumptions should fallback to the numerical comparisons.

Hence, I think that e.g. sin should have lines, somewhere, in eval that goes like:

if arg.is_zero: return S.Zero

as it right now doesn't benefit from the assumptions system in the evaluation. Maybe that is the way to solve/improve #17511 btw.

So, basically, if you want to check that if an expression (unevaluated or not) is identical to S.Zero, there are plenty of ways to do that without the assumption system.

The whole idea is really that I think all checks should be done as .is_zero rather than == 0

Given that in a parallel thread you are talking about mechanically changing the code to things that are faster I should point out the main difference here which is that is_zero can take an arbitrary amount of time. It should generally be quicker than doit (where doit might result in something more obviously zero) but it can be a lot slower than == 0.

In [17]: x = Symbol('x', positive=True) In [18]: random_poly(x, 100, -10, 10) == 0 Out[18]: False In [19]: random_poly(x, 100, -10, 10).is_zero # Try this a few times because it can be fast or slow

I'm not sure about this.

I agree. I think is_zero is mainly there to give None/False values for expressions which are not identically zero. Add(x, -x, evaluate=False).is_zero gives None and it should since x - x might be nan if x is not finite. equals should try to evaluate expressions, but assumptions should not. If the first check of foo.is_zero is if foo is S.Zero: return True then you can use is_zero instead of is S.Zero everywhere. But I would advise, with @oscarbenjamin , against trying to make the assumptions use evaluation.

oscargus · 2019-08-25T15:31:53Z

Turns out that evaluating huge factorials isn't really feasible... So I'll have to rethink this a bit...

oscarbenjamin · 2019-08-25T15:35:16Z

Turns out that evaluating huge factorials isn't really feasible... So I'll have to rethink this a bit...

This is what I meant when I said that the user probably has a good reason for using evaluate=False :).

As a general principle I think that the assumptions system should lie underneath the evaluation system. Evaluation should use the assumptions system to make decisions but the assumptions system should not use evaluation to answer queries.

smichr · 2019-08-25T17:42:20Z

the assumptions system should not use evaluation to answer queries

If this is the case then how should a query about whether some complicated expression is positive be handled when expr.is_positive is None? Maybe that is where SymPy should recommend that users use expr > 0 to get evaluation, if possible (since it might not be possible if expr cannot be evaluated with precision).

oscargus · 2019-08-26T07:32:01Z

I can see the point of not using the evaluation system in _eval_is_xx methods. However, it is worth noting that it currently is doing that in some places.

If this is the case then how should a query about whether some complicated expression is positive be handled when expr.is_positive is None? Maybe that is where SymPy should recommend that users use expr > 0 to get evaluation, if possible (since it might not be possible if expr cannot be evaluated with precision).

I think that in the code base one can see all types of ways to solve this and I think that one should be the preferred.

Btw, this PR has now changed focus to implementing better eval and _eval_is_zero for functions.

Given that in a parallel thread you are talking about mechanically changing the code to things that are faster I should point out the main difference here which is that is_zero can take an arbitrary amount of time.

As you see, in this PR I (now) first check is S.Zero (which is faster than == 0 and equivalent as long as the functions returns SymPy objects which they should...) and then later with is_zero. Except that I haven't rewritten cases that only checks with is_zero. If one can detect a zero valued expression early on that should be beneficial from an execution time perspective compared to propagating symbolic expressions.

oscargus · 2019-08-26T07:39:21Z

Btw, I'll add tests and revert the quantum fix. Although it should be relevant for consistency to be able to provide the evaluate keyword to Dagger, I cannot really figure out the problem, plus that the printing assumes that it is always evaluated. So not really worth the hassle right now.

sympy/functions/special/elliptic_integrals.py

oscarbenjamin · 2019-08-26T09:17:32Z

I can see the point of not using the evaluation system in _eval_is_xx methods. However, it is worth noting that it currently is doing that in some places.

Can you show an example?

There are cases where a subexpression is created for example if (self.args[0]/pi).is_integer: Is that what you mean?

oscargus · 2019-08-26T09:43:03Z

Search for self.func(*self.args) to see all cases. Some are potentially problematic, although none enforces evaluate=True.

Seems like log is the worst case when it comes to _is_eval_xx, e.g.

    def _eval_is_rational(self):
        s = self.func(*self.args)
        if s.func == self.func:
            if (self.args[0] - 1).is_zero:
                return True
            if s.args[0].is_rational and fuzzy_not((self.args[0] - 1).is_zero):
                return False
        else:
            return s.is_rational

But the maybe most worrying is Basic.copy:

a = Add(1, 1, evaluate=False)
a.copy() # Results in 2

Now, I'm not sure what the purpose of copy is, but I can imagine that if someone uses it, there may be unexpected consequences.

oscarbenjamin · 2019-08-26T09:59:03Z

Seems like log is the worst case when it comes to _is_eval_xx, e.g.
    def _eval_is_rational(self):
        s = self.func(*self.args)

This should be removed I think.

Now, I'm not sure what the purpose of copy is, but I can imagine that if someone uses it, there may be unexpected consequences.

There shouldn't be any need for Basic.copy since Basic is immutable. Either way though it is difficult at that level to use an evaluate kwarg because many Basic subclasses don't support it #17280 #17372.

codecov · 2019-08-26T12:07:37Z

Codecov Report

Merging #17510 into master will decrease coverage by 0.057%.
The diff coverage is 43.621%.

@@             Coverage Diff              @@
##           master    #17510       +/-   ##
============================================
- Coverage   74.76%   74.703%   -0.058%     
============================================
  Files         634       634               
  Lines      165333    165530      +197     
  Branches    38857     38937       +80     
============================================
+ Hits       123604    123656       +52     
- Misses      36309     36391       +82     
- Partials     5420      5483       +63

oscarbenjamin · 2019-08-26T12:45:10Z

sympy/functions/elementary/exponential.py


        if fuzzy_not(k.is_zero):
-            if x is S.Zero:
+            if x is S.Zero or x.is_zero:


Is it beneficial to so if x is S.Zero or x.is_zero rather than just if x.is_zero? In the case that x is S.Zero, both essentially boil down to an attribute lookup:

In [5]: x = S.Zero In [6]: %timeit bool(x is S.Zero or x.is_zero) 212 ns ± 1.97 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each) In [7]: %timeit bool(x.is_zero) 184 ns ± 5.24 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In any other case the x is S.Zero check will be False and we still end up calling x.is_zero.

Even if this was faster I would be unsure about this change: it would be better to optimise S.Zero.is_zero than pepper things like this around the codebase.

oscarbenjamin · 2019-08-26T12:48:23Z

sympy/functions/elementary/exponential.py

+    def _eval_is_zero(self):
+        x = self.args[0]
+        if len(self.args) == 1:
+            k = S.Zero


As a general principle I think it would be better to use properties like x and k than to manipulate args directly. We should try to limit the number of places that depend on the exact layout of args.

oscarbenjamin · 2019-08-26T12:54:24Z

sympy/functions/special/bessel.py

@@ -1117,6 +1117,8 @@ def eval(cls, arg):
                return S.Zero
            elif arg is S.Zero:
                return S.One / (3**Rational(2, 3) * gamma(Rational(2, 3)))
+        if arg.is_zero:
+            return S.One / (3**Rational(2, 3) * gamma(Rational(2, 3)))


Maybe the elif above isn't needed

oscarbenjamin · 2019-08-26T12:56:41Z

sympy/functions/special/bessel.py

-        if a == 0:
-            if m == 0 and b == 0:
+        if a is S.Zero:
+            if m is S.Zero and b is S.Zero:


I see why you were asking about sympification of arguments to eval. I think it's fine to treat eval as an internal routine so that sympification is the user's responsibility.

oscarbenjamin · 2019-08-26T13:07:40Z

sympy/functions/special/error_functions.py

@@ -647,6 +664,10 @@ def eval(cls, x, y):
        if isinstance(y, erf2inv) and y.args[0] == x:
            return y.args[1]

+        if x.is_zero or y.is_zero or x.is_extended_real and x.is_infinite or \
+                y.is_extended_real and y.is_infinite:
+            return erf(y) - erf(x)


There is a similar check a few lines above. Maybe this can replace that

oscarbenjamin · 2019-08-26T13:09:14Z

sympy/functions/special/error_functions.py

@@ -846,6 +874,9 @@ def eval(cls, z):
        elif z == 2:
            return S.NegativeInfinity

+        if z.is_zero:
+            return S.Infinity


We are also checking z is S.Zero above. Maybe this can replace that

oscarbenjamin · 2019-08-26T13:11:00Z

sympy/functions/special/error_functions.py

+        if x.is_zero:
+            if y.is_zero:
+                return S.Zero
+            else:


These checks are also duplicated above.

oscarbenjamin · 2019-08-26T13:11:49Z

sympy/functions/special/error_functions.py

@@ -1028,6 +1071,9 @@ def eval(cls, z):
        elif z is S.NegativeInfinity:
            return S.Zero

+        if z.is_zero:
+            return S.NegativeInfinity


This is also duplicated above

oscarbenjamin · 2019-08-26T13:13:50Z

sympy/functions/special/gamma_functions.py

@@ -285,7 +284,7 @@ def eval(cls, a, x):
        #    of both s and x), i.e.
        #    lowergamma(s, exp(2*I*pi*n)*x) = exp(2*pi*I*n*a)*lowergamma(a, x)
        from sympy import unpolarify, I
-        if x == 0:
+        if x is S.Zero:


Maybe this could be .is_zero instead of also checking that below

oscarbenjamin · 2019-08-26T13:17:13Z

sympy/functions/special/gamma_functions.py

@@ -676,16 +689,18 @@ def eval(cls, n, z):
                                return S.Infinity
                            else:
                                return S.Zero
+                        if n.is_zero:
+                            return S.Infinity


Maybe use this above instead of checking is S.Zero as well

oscarbenjamin · 2019-08-26T13:20:40Z

sympy/functions/special/zeta_functions.py

+            return S.EulerGamma
+
+        if n.is_integer == False:
+            return S.ComplexInfinity


I think you can use is False here.

oscarbenjamin · 2019-08-26T13:21:52Z

sympy/functions/special/zeta_functions.py

+            return S.ComplexInfinity
+
+        if n.is_zero and a in [None, 1]:
+            return S.EulerGamma


I'd use a tuple for [None, 1]. I believe that in CPython it can be embedded as a "compile time constant" in the pyc so a new list isn't created each time the function is called.

oscarbenjamin · 2019-08-26T13:25:44Z

This generally looks good although I left comments.

There are lots of places where we now have something like:

def eval(cls, arg):
    if arg is S.Zero:
        # Some stuff
    # Some other stuff
    if arg.is_zero:
        # Same stuff as before

I think it would be better to combine those. I presume that you do this because it will possibly be quicker but I don't think it will be because in most cases where another condition applies the is_zero check will be cheap and S.Zero.is_zero is also potentially cheaper than S.Zero is S.Zero.

oscarbenjamin · 2019-08-28T22:31:34Z

Having looked at this again I want to clarify that my comments above are just subjective comments: I think the changes here are fine to merge.

It does look like a lot of these changes are untested though. Codecov is showing 45% diff coverage which doesn't seem right but a quick scan of the diff shows some untested things like sinh(Symbol('x', zero=True))==0.

oscarbenjamin · 2019-09-24T23:51:29Z

@oscargus are you still working on this? If not I think this should be merged.

…appelf1 and hyper

oscarbenjamin · 2019-09-26T00:38:46Z

I will merge this soon if no one objects

oscargus added core functions labels Aug 25, 2019

oscargus mentioned this pull request Aug 25, 2019

sin(Add(1, -1, evaluate=False)).is_zero returns False #17511

Closed

oscargus force-pushed the iszerorational branch from 449e411 to 04e512a Compare August 25, 2019 13:58

oscarbenjamin reviewed Aug 25, 2019

View reviewed changes

oscargus force-pushed the iszerorational branch from e290d34 to 4433586 Compare August 25, 2019 22:26

oscargus mentioned this pull request Aug 26, 2019

Change S(0) to S.Zero etc #17495

Merged

oscargus changed the title ~~Implemented generic is_zero/is_rational and allowed keyword args for …~~ Better eval and is_zero checking for a number of functions. Aug 26, 2019

oscargus commented Aug 26, 2019

View reviewed changes

sympy/functions/special/elliptic_integrals.py Outdated Show resolved Hide resolved

oscargus commented Aug 26, 2019

View reviewed changes

sympy/functions/special/elliptic_integrals.py Outdated Show resolved Hide resolved

oscargus commented Aug 26, 2019

View reviewed changes

sympy/functions/special/elliptic_integrals.py Outdated Show resolved Hide resolved

oscarbenjamin reviewed Aug 26, 2019

View reviewed changes

oscargus added 7 commits September 25, 2019 13:45

Implemented generic is_zero/is_rational and allowed keyword args for …

236492e

…appelf1 and hyper

Improved zero detection for many functions

807908f

Fixed some minor typos

cfe3cc8

Fixed doctests and better evaluation

c40248b

Reverted Dagger change, fixed typos and improved zeta

a0a5eee

Another typo and some improvements

800328a

Removed gamma rewrite

0904b18

oscarbenjamin force-pushed the iszerorational branch from f706a0f to 0904b18 Compare September 25, 2019 12:59

Empty commit

c3e8f75

oscarbenjamin merged commit 2d5d978 into sympy:master Sep 28, 2019

oscargus mentioned this pull request Aug 11, 2021

Fixed #17454 and cleanup #17482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better `eval` and `is_zero` checking for a number of functions. #17510

Better `eval` and `is_zero` checking for a number of functions. #17510

oscargus commented Aug 25, 2019 •

edited

Loading

sympy-bot commented Aug 25, 2019 •

edited

Loading

oscarbenjamin Aug 25, 2019

oscargus Aug 25, 2019

oscargus Aug 25, 2019

oscargus Aug 25, 2019

oscargus Aug 25, 2019 •

edited

Loading

oscarbenjamin Aug 25, 2019

smichr Aug 25, 2019

oscargus commented Aug 25, 2019

oscarbenjamin commented Aug 25, 2019

smichr commented Aug 25, 2019

oscargus commented Aug 26, 2019

oscargus commented Aug 26, 2019

oscarbenjamin commented Aug 26, 2019

oscargus commented Aug 26, 2019 •

edited

Loading

oscarbenjamin commented Aug 26, 2019

codecov bot commented Aug 26, 2019 •

edited

Loading

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin Aug 26, 2019

oscarbenjamin commented Aug 26, 2019

oscarbenjamin commented Aug 28, 2019

oscarbenjamin commented Sep 24, 2019

oscarbenjamin commented Sep 26, 2019

Better eval and is_zero checking for a number of functions. #17510

Better eval and is_zero checking for a number of functions. #17510

Conversation

oscargus commented Aug 25, 2019 • edited Loading

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

sympy-bot commented Aug 25, 2019 • edited Loading

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

oscargus Aug 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Aug 25, 2019

oscarbenjamin commented Aug 25, 2019

smichr commented Aug 25, 2019

oscargus commented Aug 26, 2019

oscargus commented Aug 26, 2019

oscarbenjamin commented Aug 26, 2019

oscargus commented Aug 26, 2019 • edited Loading

oscarbenjamin commented Aug 26, 2019

codecov bot commented Aug 26, 2019 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscarbenjamin commented Aug 26, 2019

oscarbenjamin commented Aug 28, 2019

oscarbenjamin commented Sep 24, 2019

oscarbenjamin commented Sep 26, 2019

Better `eval` and `is_zero` checking for a number of functions. #17510

Better `eval` and `is_zero` checking for a number of functions. #17510

oscargus commented Aug 25, 2019 •

edited

Loading

sympy-bot commented Aug 25, 2019 •

edited

Loading

oscargus Aug 25, 2019 •

edited

Loading

oscargus commented Aug 26, 2019 •

edited

Loading

codecov bot commented Aug 26, 2019 •

edited

Loading