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

Conversation

elteammate
Copy link
Collaborator

Makes CDCL more accessible, especially from Java.

@elteammate elteammate requested a review from Lipen August 31, 2023 14:47
/**
* 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.

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

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


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?

@Lipen Lipen marked this pull request as draft September 14, 2023 08:29
@Lipen
Copy link
Collaborator

Lipen commented Sep 14, 2023

"Java-centric interface improvements" are postponed until we have a proper API. Superseded by #63

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