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

Improvement in satask #17144

Merged
merged 53 commits into from Aug 3, 2019
Merged

Improvement in satask #17144

merged 53 commits into from Aug 3, 2019

Conversation

@ShubhamKJha
Copy link
Member

@ShubhamKJha ShubhamKJha commented Jul 3, 2019

References to other Issues or PRs

Built over #11789.
A lot of time in the execution of satask is wasted in unnecessary creation of And and Or objects. This PR tries to remove these intermediate steps.
The current results in performance:

Tests This PR This PR (most recent) Master
test_satask 2.39 s 1.06 s 36.26 s
assumptions\tests 16.74 7.61 s 127.21

Brief description of what is fixed or changed

Other comments

Todos:

  • Fix failing tests in ask module.
  • Implement early encoding of Literals

Release Notes

  • assumptions
    • Performance of ask and satask is now improved.
    • Implemented new cnf.py to handle logical computations using low-level Python built-ins.
@sympy-bot
Copy link

@sympy-bot sympy-bot commented Jul 3, 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.

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.

<!-- 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. -->
Built over  #11789.
A lot of time in the execution of `satask` is wasted in unnecessary creation of `And` and `Or` objects. This PR tries to remove these intermediate steps.
The current results in performance:

| Tests             | This PR         | This PR (most recent)    |  Master        |
| ------------- | -------------   | -------------  | -------------|
| test_satask    | 2.39 s   | 1.06 s | 36.26 s|
| assumptions\tests | 16.74 | 7.61 s     | 127.21 |

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


#### Other comments
Todos:
* [x]  Fix failing tests in `ask` module.
* [ ]  Implement early encoding of `Literal`s


#### 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 -->
* assumptions
  * Performance of ask and satask is now improved.
  * Implemented new `cnf.py` to handle logical computations using low-level Python built-ins.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@oscargus
Copy link
Contributor

@oscargus oscargus commented Jul 6, 2019

Looks promising! However, since you base it on #11789 you should include those commits here (with correct authorship). Not sure how to go that in an easy way though, but maybe someone else knows.

@oscargus
Copy link
Contributor

@oscargus oscargus commented Jul 6, 2019

Not sure that all of these are from assumptions, but many are:

___________________________ unexpectedly fast tests ____________________________
...
test_pow_pos_neg - Took 0.588 seconds
test_zero - Took 1.123 seconds
test_negative - Took 1.177 seconds
test_bounded1 - Took 1.498 seconds
test_hermitian - Took 1.518 seconds
test_positive - Took 1.681 seconds
test_pow2 - Took 1.867 seconds
test_pow4 - Took 1.883 seconds
test_bounded3 - Took 1.955 seconds
test_real_pow - Took 2.641 seconds
test_pow3 - Took 2.760 seconds
test_bounded2a - Took 3.033 seconds
test_even_query - Took 3.134 seconds
test_issue_11238 - Took 4.158 seconds
test_odd_query - Took 4.445 seconds

Nice!

@codecov
Copy link

@codecov codecov bot commented Jul 13, 2019

Codecov Report

Merging #17144 into master will increase coverage by 0.296%.
The diff coverage is 82.524%.

@@              Coverage Diff              @@
##            master    #17144       +/-   ##
=============================================
+ Coverage   74.487%   74.784%   +0.296%     
=============================================
  Files          623       627        +4     
  Lines       161339    163574     +2235     
  Branches     37863     38629      +766     
=============================================
+ Hits        120178    122328     +2150     
- Misses       35833     35882       +49     
- Partials      5328      5364       +36

@oscarbenjamin
Copy link
Contributor

@oscarbenjamin oscarbenjamin commented Jul 13, 2019

@ShubhamKJha this looks like it will be useful but you need to fix the attribution of the commits.

The way I would do this is to start a new branch as a copy of the old PR (with previous author information intact), rebase that on current master, then apply my changes on top.

@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Jul 26, 2019

ping @asmeurer, @oscarbenjamin, @jksuom does it need further edits?

Any other features (including a good strategy for the early encoding of literals) can be implemented in future PRs, since that will require some refactoring of code at various levels.

sympy/assumptions/ask.py Outdated Show resolved Hide resolved
sympy/assumptions/ask.py Outdated Show resolved Hide resolved
@asmeurer
Copy link
Member

@asmeurer asmeurer commented Jul 26, 2019

Placing encoding on CNF object appeared to be difficult since I had implemented interactions among different CNF objects. That would have required updating the encoding every such time. Currently, I have changed Literal to be a wrapper around expressions and the encoding happens at EncodedCNF only.

I expect that might even be faster than doing __eq__ on large SymPy expressions. But we can see how this performs.

@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Jul 29, 2019

ping @asmeurer @oscarbenjamin @jksuom is it now good enough to merge?

sympy/assumptions/ask.py Outdated Show resolved Hide resolved
@jksuom
Copy link
Member

@jksuom jksuom commented Jul 29, 2019

This looks good to me.

@ShubhamKJha ShubhamKJha changed the title [WIP] Improvement in satask Improvement in satask Jul 29, 2019
@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Aug 1, 2019

The failing test seems to be unrelated to the PR here. Can anyone restart tests?

@jksuom
Copy link
Member

@jksuom jksuom commented Aug 1, 2019

Restart does not help. It seems that other pulls are also failing with an error in lambdify.

@ShubhamKJha ShubhamKJha closed this Aug 3, 2019
@ShubhamKJha ShubhamKJha reopened this Aug 3, 2019
@ShubhamKJha
Copy link
Member Author

@ShubhamKJha ShubhamKJha commented Aug 3, 2019

Ok, the tests are passing. Ping @jksuom @oscarbenjamin, this can be merged now.

@jksuom
Copy link
Member

@jksuom jksuom commented Aug 3, 2019

Thanks, I also think that this can be merged.

@jksuom jksuom merged commit a7988f0 into sympy:master Aug 3, 2019
3 checks passed
@jksuom jksuom mentioned this pull request Aug 3, 2019
4 tasks
@oscargus
Copy link
Contributor

@oscargus oscargus commented Aug 3, 2019

Thanks @ShubhamKJha (and@rlamy )! Great to see some speed-ups!

(@ShubhamKJha if you are not bored with this topic yet, feel free to remove the slow-marks for the no longer slow tests in another PR. These are probably the candidates.

537 test_negative - Took 0.450 seconds
538 test_zero - Took 0.502 seconds
539 test_positive - Took 0.575 seconds
540 test_bounded1 - Took 0.636 seconds
541 test_hermitian - Took 0.711 seconds
542 test_pow4 - Took 0.809 seconds
543 test_bounded3 - Took 0.839 seconds
544 test_real_pow - Took 0.994 seconds
545 test_pow2 - Took 1.036 seconds
546 test_even_query - Took 1.059 seconds
547 test_pow3 - Took 1.185 seconds
548 test_bounded2a - Took 1.331 seconds
549 test_odd_query - Took 1.628 seconds
550 test_imaginary - Took 1.676 seconds
551 test_bounded2b - Took 2.334 seconds
553 test_fidelity - Took 4.954 seconds

)

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Aug 3, 2019

Ah yes that definitely needs to be done, I forgot about that.

@oscargus
Copy link
Contributor

@oscargus oscargus commented Aug 7, 2019

slow-markers are removed in #17367

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

7 participants