-
Notifications
You must be signed in to change notification settings - Fork 8
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
[wip] major refactoring for perf and modularity #11
Conversation
Looks good (as long as the Travis check pass). Is there a reason for having filenames capitalized ? I usually keep all my folders/filenames lowcase because it's easier to type in the terminal (but it's not an absolute rule). |
Travis configuration and opam file should be updated to reflect the changes in ocaml versions |
I use capitalized file names these days, because it maps exactly to the module name. When using acronyms, if avoids having files named |
Questions:
|
In
The test stuff is currently hidden (and ideally not part of what is installed), but it could indeed be moved to a separate library. I didn't make it visible because I felt the code wasn't really good enough, but feel free to improve it. |
I think the major changes are there. You might want to start by looking at the API changes, and if it's ok, then look into the code itself. Note that in mc2 there are still some parts that I consider more elegant:
|
You have a |
The Otherwise: this PR is a bit old, another pass might be useful to get it ready ot be merged I think. |
I'll try and do a serious review soon (as in in a few days, one week max). @c-cube do you think you'll have some time to address the potential comments ? |
I think so, yes. I'm spending too much time on my own fork (sidekick) but well. |
rebased |
Ugh, I hate travis -_- |
- move to jbuilder - use a functorial heap (with indices embedded in lit/var) - update Vec with optims from mc2 - change semantics of Vec.shrink - use new Log module
this replaces the old "tag" system
- goal: do not keep clauses alive after they're gc'd - default is to indeed log proofs
more accurate representation of the semantics, esp. for theory lemmas
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.
Looks very good overall ! Some small comments here and there, but this is already mergeable, and we can fix things later.
Last question: why is Backtrackable_ref
part of core
and not sudoku
? It seems to only be used in the sudoku example, and even though it is very convenient for SMT solvers, I don't know if it really needs to be part of the core of msat.
src/core/Internal.ml
Outdated
(* inverse of the activity factor for variables. Default 1/0.999 *) | ||
|
||
let clause_decay : float = 1. /. 0.999 | ||
(* inverse of the activity factor for clauses. Default 1/0.95 *) |
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.
Defaults values in the comments are out of sync for var_decay
and clause_decay
.
) hyps); | ||
let export st fmt ~hyps ~history ~local = | ||
assert false | ||
(* FIXME |
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 is this commented out ? I mean, did you encounter some example that made the assertions fail ?
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.
it's because the vecs have changed, and stuff was hidden (in particular, the premises, I think).
edit: clauses are not stored as faithfully now (no more local assumptions, no storage of unit hypothesis, etc.) so this makes less sense imho
) hyps); | ||
assert false | ||
(* FIXME | ||
assert (Vec.for_all (fun c -> St.Clause.premise c = St.Hyp) hyps); |
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.
Same question. Do the changes to clauses require these functions to be changed significantly ?
- use `cid` instead of `name` for clauses - has the name of the clause, not its content - simplified some things
This reverts commit 5d7e345.
🎉 🎉 🎉 🌮 🎆 |
jbuilderdune