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

Simplify tautologies #29

Merged
merged 7 commits into from
Dec 12, 2022
Merged

Simplify tautologies #29

merged 7 commits into from
Dec 12, 2022

Conversation

haz
Copy link
Collaborator

@haz haz commented Dec 9, 2022

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).

@haz haz requested a review from blyxxyz December 9, 2022 02:53
@haz
Copy link
Collaborator Author

haz commented Dec 9, 2022

Closes #30

Copy link
Collaborator

@blyxxyz blyxxyz left a 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.

@haz
Copy link
Collaborator Author

haz commented Dec 9, 2022

Great idea! It caught a corner case where (x & ~x) | y was being simplified to just y. Can you have another look after the latest commit?

@blyxxyz
Copy link
Collaborator

blyxxyz commented Dec 9, 2022

Ah! I hadn't spotted that.

My only concern now is that the name simplify_tautologies is inaccurate because it also covers contradictions.

@haz
Copy link
Collaborator Author

haz commented Dec 10, 2022

Ah, right. Just simplify work?

nnf/__init__.py Outdated Show resolved Hide resolved
nnf/tseitin.py Outdated Show resolved Hide resolved
@haz
Copy link
Collaborator Author

haz commented Dec 12, 2022

Thanks, @blyxxyz ! Should be all set to go if you want to approve/merge.

Copy link
Collaborator

@blyxxyz blyxxyz left a 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.

@haz haz merged commit 034112a into master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants