search

bastikr / boolean.py

Implements boolean algebra in one module.
BSD 2-Clause "Simplified" License
78 stars 34 forks source link

Cached simplify() #28

Closed Kronuz closed 8 years ago

Kronuz commented 8 years ago

This avoids re-calculating the already simplified version of a boolean tree more than once. This makes for a more efficient implementation (during tests, for instance, now there are many cache hits). It also serves the purpose of mitigating what @bastikr commented in issue #27 regarding bool implementation.

bastikr commented 8 years ago

Sorry, I know that this took some effort but I really don't like this approach. Caching is a trade off between speed and memory and I feel it shouldn't be forced onto the user. The user can best gauge if it really is necessary and then it's not too hard to do it on a per case basis. Additionally it makes the code needlessly complex - I'm for keeping it as simple as possible. Do you think there are realistic use cases where caching provides a meaningful improvement?

Kronuz commented 8 years ago

Well, just for the case of bool checks... which I still strongly believe should give you True if the simplified version is True; because that means the expression, even if not simplified, is already True, and otherwise it'd throw an incorrect exception.

I'm not sure the caching brings too much to the table, expressions are going to be quite simple most of the time; but then again, what simplify() returns is going to live in another variable anyway, and in a simplified boolean tree, the cache will be shared; parent will have the _simplified object already as a child, so having this cache wouldn't be much of a memory tradeoff anyway.

Kronuz commented 8 years ago

After a discussion in pull request #27, it's been decided it's not worth it to use cache for simplify(); after all, this was for using simplify() during the boolean evaluation of expressions.