-
Notifications
You must be signed in to change notification settings - Fork 9
Memoization and other optimizations and fixes #17
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
Conversation
haz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the weakref strategy is a bit more involved than I have time to fully grok, but generally things make sense. Quite nice to see some of the major reductions in lieu of faster SAT calls & caching!
The general high-level points that came to mind:
- Best practices for when/how to use the memoization would be a helpful addition to
CONTRIBUTING.md. It's pretty sparse at the moment, but worth adding to prior to having more students brought in on the project. - It strikes me as useful to run the full test suit with/without memoization. I realize this clashes with some of the memoization mods in the tests (clearing and such), but would it possibly surface corner case bugs?
- Similarly, all of the forced deterministic settings in the testing seem like a prime place to run both with/without that optimization.
- Don't use Builder, it doesn't do much here - Clean up imports - Improve @memoize comment
Do pre-marking with a `set` method that we can patch in the tests. Run assertions both before and after `.mark_deterministic()`. Move _WeakrefMemoized to the end of the file as it's just noise for Mypy, not the actually interesting code to read.
|
- No need to specify the environment - MyPy has its own config file - MyPy runs for Python 3.9
haz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving so you can merge when set, but just noticed one minor issue in an otherwise CONTRIBUTING.md doc -- thanks!
decomposableand such arguments obsolete. Sentence properties are memoized usingweakref, which limits the amount of extra memory we need per node (compared to an attribute for each property).A
.mark_deterministic()method can be used to declare that a sentence is deterministic.Sentence properties are marked in advance when possible. This does screw with the tests a little, you have to make sure to bypass the memoization when appropriate.
.vars()in the same way (resolves Improve the vars() computation #12), and replace a lot of the internal use of.vars()by a smarter system with a temporary cache. This hugely speeds up those methods (including.decomposable()) on large sentences, because.vars()does redundant work if you run it on many nodes in the sentence. I think it takes the complexity down from quadratic to something more manageable.The memoization does mean that running
.vars()on individual nodes is inadvisable, because it associates a whole newfrozensetwith each. It's something you should try to only run on full sentences. That goes for memoized methods in general.is_CNF()andis_DNF()semantics (resolves is_CNF semantics #14)..satisfiable(), now that it uses a SAT solver as a sane baseline. This slows them down for some cases and speeds them up for others, but I think it massively improves the worst-case performance for each.@haz Can you check if you see anything that's questionable or badly explained or needs to make better use of these changes?