-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: check-all-sat support #846
Conversation
- add a data-structure for models, - save the strucutre in the SAT's env - print the models in the Frontend module
all_models: reset last saved model before exploring other branches all_models: re-set timelimit, which is inteded to be used per SAT branch/model in this case print_status: Unknown instead of Timeout if get_interpretation && why3 output continue models enumeration after a Timeout avoid infinite loop in case no progression is made in all-models enumeration do not support check-all-sat/all-models in Fun_SAT
@@ -145,7 +145,107 @@ let main worker_id content = | |||
begin match kind with | |||
| Ty.Check | |||
| Ty.Cut -> { state with local = []; } | |||
| Ty.Thm | Ty.Sat -> { state with global = []; local = []; } | |||
| Ty.Thm | Ty.Sat | Ty.AllSat _ -> |
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.
There seems to be twice the same code here. Why the duplication?
@@ -40,6 +40,7 @@ let parse_cmdline () = | |||
try Parse_command.parse_cmdline_arguments () | |||
with Parse_command.Exit_parse_command i -> exit i | |||
|
|||
|
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.
Remove this line
timeout_kind = | ||
match d.st_decl with | ||
| Assume _ | PredDef _ | RwtDef _ | Push _ | Pop _ | ThAssume _-> | ||
(* cannot raise Sat or Unknown 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.
Can't alt-ergo Timeout during one of these steps?
E.Set.add t acc | ||
)E.Set.empty l) | ||
|
||
| Thm | Sat -> |
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.
| Thm | Sat when Options.get_all_models ()
(for instance, in case of timeout and no model computed | ||
so far). We should stop to avoid infinite loop *) | ||
if timeout_kind != NoTimeout then raise Util.Timeout; | ||
(* TODO: is it the appropriate status? *) |
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 don't know, is it?
(* 2. default case + case where a simple interpretation is | ||
requested *) | ||
print_model t (Some timeout_kind); | ||
(* TODO: is it the appropriate status? *) |
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 don't know, is it?
@@ -490,7 +490,7 @@ let rec pp_value ppk ppf = function | |||
let pp_constant ppf (_sy, t) = | |||
Fmt.pf ppf "%a" SmtlibCounterExample.pp_abstract_value_of_type t | |||
|
|||
let output_concrete_model fmt m = | |||
let output_concrete_model ~pp_prop_model:_ fmt m = |
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.
Can't you remove this argument?
@@ -286,6 +286,9 @@ type goal_sort = | |||
(** The goal to be proved valid *) | |||
| Sat | |||
(** The goal to be proved satisfiable *) | |||
| AllSat of string list | |||
(** Rather generate all models involving the given list of |
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 'Rather'?
Before reviewing the code in detail, I want to better understand what we are trying to achieve with that feature, so I will only make high-level remarks here. It was my understanding that this feature was required for OptimAE. After looking at the code, it doesn't appear to be the case, as OptimAE uses a different mechanism to iterate over the propositional models for optimization (in fact, in the OptimAE branch, the As an aside, this feature was initially implemented for an unnamed intern project in 2011, dropped during the development of OptimAE (the removal was merged in #530) and then re-implemented later --- but if I had to wager, without that one-day hack in 2011, it would not have been implemented in OptimAE. With that out of the way, I am not sure we want to merge this at all. I don't think there is much value in enumerating propositional models rather than actual models in general (in part because having a propositional model does not guarantee that a corresponding model of the input formula exists), and if we care about models, then model enumeration can be implemented outside the solver. The typical way to implement model enumeration uses incremental solving: first get a model using I agree that providing this loop in a built-in way would be useful (in fact, the current implementation uses the exact loop I described above), but given that there is no rush for the feature I think it would be better to first clean up the interface to be able to do proper incremental solving (rather than giving a single list of commands and we're done, as we discussed) and to implement this feature on top of the incremental solving API once it exists (I think the relevant issue is #382 ). Edit: as a side note, we should all this |
I agree to postpone this feature until we have a sufficient support for incremental mode. |
I think we can close this PR. Keeping it up to date is painful and we do not plan to support this feature in the near future. |
agreed |
This PR adds the support for the
check-all-sat
support. I think we can merge the PR without clean-up its implementation as the feature doesn't break the classical model generation. Please see #847 for a clean-up roadmap of this feature.The PR includes two ways to use this new feature:
--all-sat
(or--all-models
), AE produces all the possible propositional models after eachcheck-sat
.For instance using the input file:
and the command
dune exec -- alt-ergo --all-sat --produce-models --sat-solver CDCL test.smt2
, we got the output:output of the command
check-all-sat
is available in the SMT-LIB input language only. For instancewith the command
dune exec -- alt-ergo --produce-models --sat-solver CDCL --frontend legacy test.smt2
outputs
output of the commad
The PR includes also another option to print a boolean model while printing the first-order model. The feature is broken but I can restore it easily. I'm not sure if the option is really useful, at least while using the SMT-LIB input language, as there is a standard way to obtain this feature (in fact a more general feature), see #844. We should deprecate the option or reserve it for the native input language.
Some important remarks about the feature:
tableaux
solver. The reason is simple and I actually ran into this issue while testing outputted models with the Tableaux solver. I have a solution that I will explain in details in an appropriate issue.unsat
at the end because it exhausted all the possible boolean models. This answer leaks the way AE works under the hood. I guess the best solution consists to print a comment telling we have exhausted all the boolean models. But fixing this issue will be easier after fixing Bubble decision of printing models upwards #833.