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

Small refactoring of partition information #219

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

blishko
Copy link
Member

@blishko blishko commented Jan 23, 2021

This PR proposes moving the partition information of clauses directly to the proof structure, as this is needed only for building resolution proof for interpolation.
This change allows to clean up the interface of adding clauses to the SAT solver and removes the dependency of Cnfizer on PartitionManager.

@blishko blishko requested a review from aehyvari January 23, 2021 16:12
@blishko blishko force-pushed the partition-refactor branch from 4ec9391 to b6b90d6 Compare January 25, 2021 15:49
Martin Blicha added 2 commits January 29, 2021 12:10
This cleans up Cnfizer a bit, because it does not need to know about the
partitioning anymore.
@blishko blishko force-pushed the partition-refactor branch from b6b90d6 to 6806db7 Compare January 29, 2021 11:10
Copy link
Member

@aehyvari aehyvari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

I'm thinking about the interface though. Could we pass the partitionIndex as an argument to the newSMTClause methods in {Core,Simp}SMTSolver, and therefore also at insertFormula in MainSolver? I think it would be good not to rely on the state in the SMT solver. For instance the partitions of true and false are not very explicit in the current implementation, and the partitions of the satelite-simplified clauses (which we hopefully are not doing) would be explicit. This was probably an issue in the old code already, but could be good to discuss at this PR. I made quickly the change on my local working directory, and could commit it here if you want.

@@ -1044,6 +1035,12 @@ void CoreSMTSolver::finalizeProof(CRef finalConflict) {
proof->endChain(CRef_Undef);
}

void CoreSMTSolver::setPartition(std::size_t partitionIndex) {
if (proof) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to logsProofForInterpolation()?

{
assert(decisionLevel() == 0);
inOutCRefs = {CRef_Undef, CRef_Undef};
if (!isOK()) { return false; }
bool logsProofForInterpolation = this->logsProofForInterpolation();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This old code seems a little redundant. Maybe we could fix it on this commit as well.

@blishko
Copy link
Member Author

blishko commented Feb 1, 2021

I agree that the interface is not entirely clear.
The partitioning is only needed for interpolation and needs to be synchronized with the interpolation interface where user specifies the binary partitioning. The question is how to keep in sync what the user thinks forms a partition of given index and what solvers thinks about which clauses belong to partition of the given index.
There is exactly this problem of where the clauses true and not false should belong, or any of these 'satellite clauses' (which are now disabled).

For some reason I don't like the idea of explicitly passing the index in the addClause methods, as under normal circumstances, the solver does not care about the partitions.

If you have some proposal, maybe commit to a separate branch and send a link?

Now I feel that it might be good to change the interpolation interface to not use ipartitions_texplicitly, but use something along the interpolation standard proposal, to use the formulas directly to specify the partitioning. This would free us to implement the actual handling of partitioning in different ways.

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