Skip to content

Commit

Permalink
Acquire problem vars when necessary.
Browse files Browse the repository at this point in the history
* Ensure that all relevant problem variables are known to the master
  solver when creating clauses, weight-, and minimize-constraints.
  • Loading branch information
BenKaufmann committed Dec 7, 2019
1 parent fdd37e2 commit 13ae0ea
Show file tree
Hide file tree
Showing 8 changed files with 37 additions and 9 deletions.
3 changes: 2 additions & 1 deletion clasp/solver.h
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 +782,8 @@ class Solver {
SolverStrategies& strategies() { return strategy_; }
bool resolveToFlagged(const LitVec& conflictClause, uint8 vflag, LitVec& out, uint32& lbd) const;
void resolveToCore(LitVec& out);
void acquireProblemVars();
void acquireProblemVar(Var var);
void acquireProblemVars() { acquireProblemVar(numProblemVars()); }
//@}
private:
struct DLevel {
Expand Down
6 changes: 2 additions & 4 deletions src/clause.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,8 @@ ClauseRep ClauseCreator::prepare(Solver& s, const Literal* in, uint32 inSize, co
uint32 abst_w1 = 0, abst_w2 = 0;
bool simplify = ((flags & clause_force_simplify) != 0) && inSize > 2 && outMax >= inSize;
Literal tag = ~s.tagLiteral();
Var vMax = s.numProblemVars() > s.numVars() ? std::max_element(in, in + inSize)->var() : 0;
if (!s.validVar(vMax)) { // Handle any uncommitted vars
s.acquireProblemVars();
}
Var vMax = s.numProblemVars() > s.numVars() && inSize ? std::max_element(in, in + inSize)->var() : 0;
s.acquireProblemVar(vMax);
for (uint32 i = 0, j = 0, MAX_OUT = outMax - 1; i != inSize; ++i) {
Literal p = in[i];
uint32 abst_p = watchOrder(s, p);
Expand Down
2 changes: 1 addition & 1 deletion src/minimize_constraint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -717,7 +717,7 @@ MinimizeBuilder::SharedData* MinimizeBuilder::createShared(SharedContext& ctx, c

MinimizeBuilder::SharedData* MinimizeBuilder::build(SharedContext& ctx) {
POTASSCO_REQUIRE(!ctx.frozen());
if ((!ctx.ok() || !ctx.master()->propagate()) || empty()) {
if (!ctx.ok() || (ctx.master()->acquireProblemVars(), !ctx.master()->propagate()) || empty()) {
clear();
return 0;
}
Expand Down
1 change: 1 addition & 0 deletions src/shared_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,7 @@ Solver& SharedContext::startAddConstraints(uint32 constraintGuess) {
}
bool SharedContext::addUnary(Literal x) {
POTASSCO_REQUIRE(!frozen() || !isShared());
master()->acquireProblemVar(x.var());
return master()->force(x);
}
bool SharedContext::addBinary(Literal x, Literal y) {
Expand Down
7 changes: 4 additions & 3 deletions src/solver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -379,9 +379,10 @@ Var Solver::pushAuxVar() {
return aux;
}

void Solver::acquireProblemVars() {
if (!shared_->frozen() && numProblemVars() > numVars())
shared_->startAddConstraints();
void Solver::acquireProblemVar(Var var) {
if (validVar(var) || shared_->frozen() || numProblemVars() <= numVars() || !shared_->ok())
return;
shared_->startAddConstraints();
}

void Solver::popAuxVar(uint32 num, ConstraintDB* auxCons) {
Expand Down
4 changes: 4 additions & 0 deletions src/weight_constraint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ namespace Clasp {
// return: achievable weight
// post : lits is sorted by decreasing weights
WeightLitsRep WeightLitsRep::create(Solver& s, WeightLitVec& lits, weight_t bound) {
// Step 0: Ensure s has all relevant problem variables
if (s.numProblemVars() > s.numVars() && !lits.empty()) {
s.acquireProblemVar(std::max_element(lits.begin(), lits.end())->first.var());
}
// Step 1: remove assigned/superfluous literals and merge duplicate/complementary ones
LitVec::size_type j = 0, other;
const weight_t MAX_W= std::numeric_limits<weight_t>::max();
Expand Down
15 changes: 15 additions & 0 deletions tests/minimize_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,21 @@ TEST_CASE("Model-guided minimize", "[constraint][asp]") {
REQUIRE(data->lits[0].second == 0);
REQUIRE(data->weights.size() == 1);
}
SECTION("testNewVariableAfterStartAddConstraints") {
Literal z = posLit(ctx.addVar(Var_t::Atom));
mb.add(0, WeightLiteral(z, 1));
REQUIRE_FALSE(ctx.master()->validVar(z.var()));
SECTION("assigned") {
ctx.addUnary(z);
REQUIRE(ctx.master()->validVar(z.var()));
data = mb.build(ctx);
}
SECTION("unassigned") {
data = mb.build(ctx);
REQUIRE(ctx.master()->validVar(z.var()));
}
REQUIRE(data);
}

SECTION("testOneLevelLits") {
ctx.addUnary(c);
Expand Down
8 changes: 8 additions & 0 deletions tests/weight_constraint_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ TEST_CASE("Cardinality constraints", "[constraint][pb][asp]") {
REQUIRE(solver.isFalse(lits[3]));
REQUIRE(solver.isFalse(lits[4]));
}
SECTION("testIntegrateNewVars") {
Literal f = posLit(ctx.addVar(Var_t::Atom));
lits[2] = f;
lits.resize(3);
REQUIRE_FALSE(ctx.master()->validVar(f.var()));
REQUIRE(newCardinalityConstraint(ctx, lits, 1));
REQUIRE(ctx.master()->validVar(f.var()));
}

SECTION("test propagate") {
LitVec assume, expect;
Expand Down

5 comments on commit 13ae0ea

@rkaminsk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you forgot to call acquireProblemVars if the literal W of the weight constraint is a newly added literal.

@rkaminsk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is the best place to add this. I put it in WeightLitsRep::propagate in the patch below because it is the first function called by all create functions. Putting it in create/doCreate might be more obvious though. You decide. 😉

diff --git a/src/weight_constraint.cpp b/src/weight_constraint.cpp
index 8b8381a..81201d2 100644
--- a/src/weight_constraint.cpp
+++ b/src/weight_constraint.cpp
@@ -117,6 +117,7 @@ WeightLitsRep WeightLitsRep::create(Solver& s, WeightLitVec& lits, weight_t boun
 
 // Propagates top-level assignment.
 bool WeightLitsRep::propagate(Solver& s, Literal W) {
+	s.acquireProblemVar(W.var());
 	if      (sat())  { return s.force(W); } // trivially SAT
 	else if (unsat()){ return s.force(~W);} // trivially UNSAT
 	else if (s.topValue(W.var()) == value_free) {

@BenKaufmann
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you forgot to call acquireProblemVars if the literal W of the weight constraint is a newly added literal.

You are right. Good catch 👍

@BenKaufmann
Copy link
Contributor Author

@BenKaufmann BenKaufmann commented on 13ae0ea Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what is the best place to add this. I put it in WeightLitsRep::propagate in the patch below because it is the first function called by all create functions. Putting it in create/doCreate might be more obvious though. You decide. wink

It belongs into doCreate. Fixed in 05814ad

@rkaminsk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, now my tests will run through.

Please sign in to comment.