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] Improve satask() performance #11789

Merged
merged 15 commits into from Aug 3, 2019
Merged

[WIP] Improve satask() performance #11789

merged 15 commits into from Aug 3, 2019

Conversation

@rlamy
Copy link
Member

@rlamy rlamy commented Oct 31, 2016

This branch makes satask() about 20 times faster. There are no real algorithmic changes, this simply uses explicit data structures to avoid expensive operations such as And instantiations, to_cnf() or known_facts.rcall(). The bottleneck now appears to be converting the "relevant facts" to CNF.

To do:

  • Stabilise and clean up the new APIs
  • Add unit tests for them
  • Fix test failures
  • Worry about backwards compatibility
@asmeurer
Copy link
Member

@asmeurer asmeurer commented Nov 1, 2016

The changes look fine at a glance. I can't comment much on the changes to the DPLL code. I guess the sorting in And makes instantiating And very slow.

I don't think any of the APIs that you changed are public, so I wouldn't worry about backwards compatibility. This whole code is supposed to be experimental anyway, so we should feel free to change it and its API as much as is needed.

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Nov 1, 2016

Re: bottleneck in converting to CNF.

I haven't done any benchmarking, but I suspect we should abstract better in sathandlers.py to always return CNF clauses. We may want to "compile" them somehow. I do want to keep the facts themselves (the big for loop at the bottom) as readable as possible (it could be even potentially more readable, e.g., a pattern matching API had been suggested), but there are also opportunities to avoid duplicate computation. Pure compilation is more difficult than what the old assumptions do because of the generality of the facts (e.g., they apply to n argument functions like Mul where n is any integer).

@asmeurer
Copy link
Member

@asmeurer asmeurer commented Nov 1, 2016

The test failures here definitely need to be fixed. I do seem to recall adding sufficient test cases for this code that I'd be fairly confident that it still works correctly once the tests are passing.

@ShubhamKJha ShubhamKJha mentioned this pull request Jul 3, 2019
2 tasks
ShubhamKJha added a commit to ShubhamKJha/sympy that referenced this issue Jul 18, 2019
@jksuom jksuom merged commit d165d2e into sympy:master Aug 3, 2019
1 check failed
@sympy-bot
Copy link

@sympy-bot sympy-bot commented Aug 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.

There was an issue with the release notes. Please do not close this pull request; instead edit the description after reading the guide on how to write release notes.

  • The <!-- BEGIN RELEASE NOTES --> block was not found

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.

This branch makes satask() about 20 times faster. There are no real algorithmic changes, this simply uses explicit data structures to avoid expensive operations such as `And` instantiations, `to_cnf()` or `known_facts.rcall()`. The bottleneck now appears to be converting the "relevant facts" to CNF.

To do:
- [ ] Stabilise and clean up the new APIs
- [ ] Add unit tests for them
- [ ] Fix test failures
- [ ] Worry about backwards compatibility

@sympy-bot
Copy link

@sympy-bot sympy-bot commented Aug 3, 2019

🚨🚨🚨ERROR🚨🚨🚨 There was an error automatically updating the release notes. Normally it should not have been possible to merge this pull request. You might want to open an issue about this at https://github.com/sympy/sympy-bot/issues.

In the meantime, you will need to update the release notes on the wiki manually.

The error message was: The pull request was merged even though the release notes bot had a failing status.

@jksuom
Copy link
Member

@jksuom jksuom commented Aug 3, 2019

It seems that the release notes were updated via #17144.

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