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

Improvements of CDCL public interface #59

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ fun main() {
// Create the instance of KoSAT solver:
val solver = CDCL()

// Allocate two variables:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Auto-allocation of variable in newClause is an implementation detail, as newVariable docs clearly say. Do not teach users to rely on such behavior, but do teach them to safely pre-allocate all the used variables.

solver.newVariable()
solver.newVariable()

// Encode the TIE-SHIRT problem:
solver.newClause(-1, 2)
solver.newClause(1, 2)
Expand Down
25 changes: 24 additions & 1 deletion kosat-core/src/commonMain/kotlin/org/kosat/CDCL.kt
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,11 @@ class CDCL {
*
* The [newClause] technically adds variables automatically,
* but sometimes not all variables have to be mentioned in the clauses.
*
* Returns 1-based index of the new variable, or, equivalently, number of
* variables in the solver after the call.
*/
fun newVariable() {
fun newVariable(): Int {
// Watch
watchers.add(ClauseVec())
watchers.add(ClauseVec())
Expand All @@ -126,6 +129,8 @@ class CDCL {

// Phase saving heuristics
polarity.add(LBool.UNDEF)

return assignment.numberOfVariables
}

/**
Expand All @@ -138,6 +143,9 @@ class CDCL {
return assignment.value(lit)
}

/**
* Adds a new clause to the solver with the given literals in DIMACS format.
*/
fun newClause(vararg lits: Int) {
newClause(Clause(LitVec(lits.map { Lit.fromDimacs(it) })))
}
Expand Down Expand Up @@ -212,6 +220,21 @@ class CDCL {
*/
private var assumptions: LitVec = LitVec()

/**
* Solves the CNF problem using the CDCL algorithm.
*/
// Overload for Java
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overloads for Java can be automatically generated via @JvmOverloads

Copy link
Collaborator

Choose a reason for hiding this comment

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

However, I agree that if we make two different solve methods (with List<Lit> and with vararg Int args), we should create a separate solve() no-arg method, since annotating both methods with @JvmOverloads would produce slashing methods. Alternatively, since solve(List<Lit> = emptyList) method could be (arguably) considered the main one, only it should be annotated with JvmOverloads, so that solve() is not needed at all.

fun solve(): SolveResult {
return solve(emptyList())
}

/**
* Solves the CNF problem using the CDCL algorithm.
*/
fun solve(vararg assumptions: Int): SolveResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very misleading interface, since it uses DIMACS numeration (1-based signed literals). Currently (after my CNF rework), this is probably the only place when CDCL uses anything dimacs-related. Ultimately, this should not be the case -- it is better to stick to one Lit-numeration everywhere.

If you still want dimacs-lit solve method, make an extension, probably even call it solveWithDimacsAssumptions(vararg assumptions: Int).

return solve(assumptions.map { Lit.fromDimacs(it) })
}

/**
* Solves the CNF problem using the CDCL algorithm.
*
Expand Down
29 changes: 29 additions & 0 deletions kosat-core/src/jvmTest/kotlin/org/kosat/Example.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.kosat

import org.junit.jupiter.api.Assertions
import kotlin.test.Test

internal class Example {
@Test
fun testExample() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you removed this test from smoke tests?

// Create the instance of KoSAT solver:
val solver = CDCL()

// Encode the TIE-SHIRT problem:
solver.newClause(-1, 2)
solver.newClause(1, 2)
solver.newClause(-1, -2)
// solver.newClause(1, -2) // UNSAT with this clause

// Solve the SAT problem:
val result = solver.solve()
println("result = $result") // SAT

// Get the model:
val model = solver.getModel()
println("model = $model") // [false, true]

Assertions.assertEquals(SolveResult.SAT, result)
Assertions.assertEquals(listOf(false, true), model)
}
}