-
Notifications
You must be signed in to change notification settings - Fork 52
Develop a common proof format and export proofs #458
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
base: master
Are you sure you want to change the base?
Develop a common proof format and export proofs #458
Conversation
…getProof() in BasicProverEnvironment.java
…aviour of getProof to return unsupported operation.
…p_a_common_proof_format_and_export_proofs
…ts from SMTInterpol to a ResolutionProofDAG. Not working. Class SmtInterpolProofNode should help transform from the internal SMTInterpol proof representation to ResolutionProofDAG. Not working. Added some tests to evaluate proofs given back by SMTInterpol and the ResolutionProofDAG objects that should be returned.
Option requireProofs must be set to true through Configuration. Added a class for testing the proof generation of Z3. SMTInterpol: some documentation.
Various changes to the ProofDAG interface and all classes inheriting it.
Various changes to the ProofDAG interface and all classes inheriting it.
…o multiple resolution steps.
…n now be converted to resolution proof rules.
- Renamed some classes for clarity - Z3: Made proof traversal non-recursive for efficiency.Added tests for proof-related functionality - CVC5: Implemented necessary classes for proof production - Improved documentation - Added test for retrieving proofs in ProverEnvironmentTest.java
…entation for when no formula is available.
src/org/sosy_lab/java_smt/solvers/z3/Z3NonRecursiveProofProcessor.java
Outdated
Show resolved
Hide resolved
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.
Some general things that i noticed (some may be mentioned in change requests, but apply generally):
-
Tests should be generalized and moved into the test package if they are not using native API. You can still have tests for a single solver in the test package by using the
assume
API and only allowing a single one. The name of the solver should be mentioned in the test name then. -
Please add documentation to the new classes and interfaces. Especially mention which ones are used internally and which ones are public/usable by users and the purpose/background of the classes/interfaces.
-
You can have classes within classes, which is especially helpful to structure smaller components, e.g. nodes of a graph, within the context you are using them in. That makes the code much easier to read and structures it better.
-
Please avoid pushing print-outs or commented out code. If you are working on a feature thats not yet working, thats fine. Document it and don't use the component. Please add some documentation to the commented out code. Print-outs should never be pushed. Try using the logger or exceptions. In tests please use assertion instead of printouts. (It is OK to expect the current static output of a solver. If the test fails in the future, we devs are notified about changes that we might need to consider.)
-
Native API tests that just test that proofs work are good and can be added for all solvers supporting proofs.
* | ||
* @author Gabriel Carpio | ||
*/ | ||
public class ResProofRule { |
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.
You could add sources (publication(s), reference sites etc.), the solvers that support this natively, as well as the usage/logic.
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.
I added some documentation and will wait for further information on the format form Mathsat5 as it seems to be very similar.
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.
As discussed, we will add this as far as possible and wait for more details from the MathSAT5 devs.
public class ResolutionProofDag extends AbstractProofDag { | ||
|
||
/* | ||
public ResolutionProofDAG() { |
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.
Is this class being worked on? If yes, in what form?
Please try to not push commented out code if possible. Also, please document this if it is supposed to be used in the future.
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.
Made some adjustments to this.
try { | ||
throw new UnsupportedOperationException("Proof generation isn't enabled."); | ||
} catch (UnsupportedOperationException e) { | ||
System.out.println(e.getMessage()); |
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.
You can check if the prover is closed first, making sure that the prover is available.
It does not make much sense to throw an exception and catch it immediately again ;D
And please don't push print-outs. Try using the logger, exceptions if you want to notify the user.
Also, "Proof generation is not available for the chosen solver" is a better message for the exception type.
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.
To check the state I will move this method to AbstractProver
. There, there also a method checkGenerateProofs
that mimics the behavior of the other check methods, I understand this should throw an Exception already. Aside from this, there is still the issue that, for example, for Z3 proof generation must be set as an Option
in the configuration, so the solver should retrieve this information from the context in this case.
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.
Since you have to start proof generation in the proofer anyways, why not just getProof()
that then does the checks?
import org.sosy_lab.java_smt.ResProofRule.ResAxiom; | ||
|
||
public class ProofFactory { | ||
public static ProofNode createSourceNode(ResAxiom rule, Formula formula) { |
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.
Building a proof should not be exposed to the user. Is ProofFactory
available for users?
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.
I'm currently trying to utilize this class to eliminate classes like Mathsat5ProofProcessor
and Z3ToResoluteProofConverter
. However, I'm facing an issue: ideally, the user shouldn't have direct access to these methods and should instead retrieve proofs via getProof
from a ProverEnvironment
instance. The challenge is that making the class non-public complicates its use within the solvers' packages.
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.
In Java the following works: you expose public API in an interface. This is then implemented by a abstract class that has a second method for each public method that is not public (e.g. public ... getChildren()
and protected ... getChildren0()
) that is always called by the final default implementation of the public method. The non-public method throws a exception per default and is overridden by the solvers. Other methods needed to manipulate or build the proof are also part of the abstract class, but never public.
|
||
|
||
//String proofStr = proof.toString(); | ||
System.out.println(invokeGetProofMode(smtinterpol).toString()); |
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.
Please use assertions instead of printouts.
ShutdownManager shutdown = ShutdownManager.create(); | ||
|
||
// Create new context with SMTInterpol | ||
context = Z3SolverContext.create( |
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.
Why not use the SolverContextFactory
?
} | ||
|
||
private static Solver createEnvironment() throws CVC5ApiException { | ||
Solver newSolver = new Solver(); |
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.
This tests nothing currently. Also, native proof tests are good, but should be part of the CVC5 native test-class.
@@ -79,6 +85,7 @@ public void createEnvironment() { | |||
long cfg = msat_create_config(); | |||
|
|||
msat_set_option_checked(cfg, "model_generation", "true"); | |||
msat_set_option_checked(cfg, "proof_generation", "true"); |
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.
Why enable proofs for all tests and then build a new, dedicated solver environment for the proof test?
src/org/sosy_lab/java_smt/solvers/z3/Z3ToResoluteProofConverter.java
Outdated
Show resolved
Hide resolved
@leventeBajczi could you spare a look at this PR and maybe give us some feedback on the current proof interface? |
…r by the name of the rule
…bproof>''. The signatures of 'addEdge' and 'addChild' changed accordingly
…gh 'checkGenerateProofs()', whether the solver is closed and whether the last evaluation was UNSAT
…sserting true and false, and when previously having evaluated once for multiple assertions and retrieving a proof, then asserting different formulas, evaluating and requesting a proof again.
…ly in the last changes to it.
…ly in the last changes to it.
…ed through 'checkGenerateProofs'
…ational and UF proof, test proof for only asserting false/true, test getting 2 different proofs with same prover instance.
…and transformed into JavaSMT proofs. There is no apparent way to retrieve a formula just from a String.
…cked warning remains in `OpenSMTProof` but is suppressed
…eck that some proof is returned.
… can't produce BV proofs with it enabled
…ception is thrown when requestion a proof for asserting `false`. Took MathSAT5 out of `proofOfFalseTest` test.
…d_export_proofs' into develop_a_common_proof_format_and_export_proofs
…ilable and throw a corresponding Exception.
Objective
This PR aims to expose proofs produced by supported solvers through the API, primarily via a
getProof()
method.Key Changes
ProofNode
,ProofDag
, andProofRule
as the main components for handling proofs.Proof Format Conversion
To unify proof representation, this PR investigates converting solver-specific proofs into a common format, with RESOLUTE (used by SMTInterpol) as a candidate.
Z3ToResoluteProofConverter
provides an example of this transformation.Task List
Implement
getProof()
for Z3.Introduce
ProofNode
,ProofDag
, andProofRule
.Implement
getProof()
for other solvers.Test
getProof
more robustly.Next Steps
ProofDag
interface.