-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add support for model minimization and maximization #171
Conversation
This LGTM. You'll need to add yourself to .larabot.conf in a separate PR for the larabot presubmit to run though. |
7944f76
to
243190c
Compare
optimizer.assertCnstr(softClause, 1) | ||
optimizer.check(Model) match { | ||
case SatWithModel(model) => | ||
assert(getIntegerValue(model, v) > 10) |
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 relaxed the tests so that they work with all valid results.
I added myself to the trust list, but the CI still hasn't run. Should I just open a new PR? |
Due to the log4j vulnerability, EPFL restricted internet access to some of its servers, which seems to include larabot. |
@@ -25,7 +25,7 @@ trait SMTLIBDebugger extends SMTLIBTarget { | |||
/* Printing VCs */ | |||
protected lazy val debugOut: Option[java.io.FileWriter] = { | |||
implicit val debugSection = DebugSectionSMT | |||
if (reporter.isDebugEnabled) { | |||
if (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.
Please revert.
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.
Done.
optimizer.minimize(x) | ||
optimizer.check(Model) match { | ||
case SatWithModel(model) => | ||
println(model) |
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 think it's good practice to have println
in tests. (Same remark for the binary trees test below.)
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.
Done.
val optimizer = factory.getNewSolver() | ||
try { | ||
optimizer.assertCnstr(clause) | ||
optimizer.minimize(E(sizeId)(IntegerType())(aTree)) |
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.
Optional: could you replace minimize(x)
by maximize(-x)
to test that function as well?
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.
Done (added a new test).
676272f
to
b482847
Compare
b482847
to
eb8329b
Compare
When running the tests twice with
Could this problem have been introduced with this PR, or is it an existing issue with Inox, Scala Z3 or Z3 Java API? |
There are some issues with the native Z3 implementation which cause it to segfault once in a while (rarely and non-deterministically). Is the crash reproducible in your case? With some luck, updating to a newer Z3 version (epfl-lara/ScalaZ3#80) will help with these crashes. |
Yes; it always crashes on the second run of Log
|
Hmm, well looking at the |
@@ -17,7 +17,7 @@ trait NativeZ3Optimizer extends AbstractUnrollingOptimizer with Z3Unrolling { se | |||
|
|||
override val name = "native-z3-opt" | |||
|
|||
protected object underlying extends { | |||
protected val underlying = NativeZ3Solver.synchronized(new { |
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 should probably share the object on which synchronization is performed between
NativeZ3Solver
andNativeZ3Optimizer
.
So would it make sense to just use NativeZ3Solver.synchronized
in both classes?
I tried it (see above). It does not fix the problem. Actually I see that I get errors with other tests as well, also in
Or (failing on second run):
So this is probably unrelated to this PR. Probably best investigated in a separate issue or PR. |
The I've never seen the error you're getting in the |
Ok, thanks for taking a look! So could we merge this PR or are other changes required? |
Yep, LGTM. Thanks for addressing all the comments! |
This PR adds support for model minimization and maximization by adding
minimize
andmaximize
methods to theOptimizer
trait.The new tests can be run from the sbt shell using
it:testOnly *OptimizerSuite*
.