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

add assumption benchmark of core #73

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RituRajSingh878
Copy link
Contributor

I have added assumption benchmarks. I will add a few more commits to the assumption bench.
Then we can merge it. Also, if anything you want to modify in the code, then please tell.

self.z_0.is_commutative is True
self.z_0.is_integer is True
self.z_0.is_rational is True
self.z_0.is_algebraic is True
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to test all of these. Benchmarks should be as narrow as possible (they should time precisely one thing). Also After checking z_0.is_integer the results of the other operations will be cached for any Basic instance anyway e.g.:

In [1]: a = Add(2, 2, evaluate=False)                                                                                                          

In [2]: a._assumptions                                                                                                                         
Out[2]: {}

In [3]: a.is_integer                                                                                                                           
Out[3]: True

In [4]: a._assumptions                                                                                                                         
Out[4]: 
{'integer': True,
 'algebraic': True,
 'rational': True,
 'hermitian': True,
 'commutative': True,
 'irrational': False,
 'complex': True,
 'infinite': False,
 'imaginary': False,
 'transcendental': False,
 'finite': True,
 'extended_real': True,
 'noninteger': False,
 'real': True}

@oscarbenjamin
Copy link
Contributor

The examples you have picked mostly have precomputed cached assumptions:

In [5]: S.One._assumptions                                                                                                                     
Out[5]: 
{'commutative': True,
 'real': True,
 'imaginary': False,
 'infinite': False,
 'complex': True,
 'hermitian': True,
 'finite': True,
 'extended_real': True,
 'integer': True,
 'algebraic': True,
 'rational': True,
 'irrational': False,
 'transcendental': False,
 'noninteger': False,
 'zero': False,
 'extended_nonzero': True,
 'nonzero': True,
 'even': False,
 'odd': True,
 'extended_negative': False,
 'negative': False,
 'extended_positive': True,
 'nonpositive': False,
 'extended_nonpositive': False,
 'extended_nonnegative': True,
 'positive': True,
 'nonnegative': True}

It's reasonable to time the performance of this but I don't think we would need to do it exhaustively. It would be better to time some things that are slow or have the potential to be slow e.g.:

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

In [18]: p = random_poly(x, 10, -10, 10)                                                                                                       

In [19]: p                                                                                                                                     
Out[19]: 
     10      9      8      7      5      4      3      2          
- 4x   + 8x  + 9x  + 3x  + 5x  - 7x  - 6x  - 8x  + 2x + 1

In [20]: %time p.is_positive                                                                                                                   
CPU times: user 396 ms, sys: 1.68 ms, total: 398 ms
Wall time: 403 ms

In [21]: %time p.is_positive                                                                                                                   
CPU times: user 8 µs, sys: 0 ns, total: 8 µs
Wall time: 13.1 µs

Note that the second run of is_positive here is instantaneous because the result is cached on the instance:

In [22]: p._assumptions                                                                                                                        
Out[22]: 
{'positive': None,
 'finite': True,
 'infinite': False,
 'extended_positive': None,
 'extended_real': True,
 'commutative': True,
 'imaginary': False,
 'real': True,
 'complex': True,
 'hermitian': True,
 'extended_nonpositive': None,
 'negative': None,
 'extended_negative': None,
 'zero': None,
 'odd': None,
 'irrational': None,
 'rational': None,
 'algebraic': None,
 'integer': None,
 'extended_nonnegative': None}

@RituRajSingh878
Copy link
Contributor Author

Okay, got it. I can close this pr, or may I will keep only one benchmark from each function. But I think it is not necessary to benchmark these assumptions. So whatever suitable you can tell me.

@oscarbenjamin
Copy link
Contributor

I think it's good to keep a small number of these benchmarks just to make sure that something like S.Zero.is_zero doesn't happen to slow down. I would make a parameterised test that tries a few different values like:


class TimeAssumptionsSingleton:
    
    params = ('Half', 'Infinity', 'Exp1', ...)

    def setup(self, name):
        self.singleton = getattr(S, name)

    def time_is_zero(self):
        self.singleton.is_zero

    def time_is_positive(self):
        self.singleton.is_positive

@asmeurer
Copy link
Member

asmeurer commented May 8, 2020

There's also no point to doing the is False or is True.

@RituRajSingh878
Copy link
Contributor Author

I think it's good to keep a small number of these benchmarks just to make sure that something like S.Zero.is_zero doesn't happen to slow down. I would make a parameterised test that tries a few different values like:


class TimeAssumptionsSingleton:
    
    params = ('Half', 'Infinity', 'Exp1', ...)

    def setup(self, name):
        self.singleton = getattr(S, name)

    def time_is_zero(self):
        self.singleton.is_zero

    def time_is_positive(self):
        self.singleton.is_positive

ok, I will try to add like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants