-
Notifications
You must be signed in to change notification settings - Fork 9
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
Simplify tautologies #29
Conversation
Closes #30 |
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.
LGTM.
Another solution might be to give .model_count()
a names
argument so it can adjust for excluded names. But this is simpler and more broadly useful.
You could add a hypothesis test case that runs .to_CNF(simplify_tautologies=False)
and checks that no names were lost.
Great idea! It caught a corner case where |
Ah! I hadn't spotted that. My only concern now is that the name |
Ah, right. Just |
Co-authored-by: Jan Verbeek <[email protected]>
Thanks, @blyxxyz ! Should be all set to go if you want to approve/merge. |
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.
Looks good! Thanks for bearing with me.
Idea is to allow for tautologies in
to_CNF
conversions so that model counts aren't thrown off (variables mentioned in a tautological clause will be missing from the compilation).