Skip to content
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

[BUG] Set constraint setLe and setLt do not behave as expected #936

Closed
dimitri-justeau opened this issue Sep 22, 2022 · 16 comments
Closed
Labels

Comments

@dimitri-justeau
Copy link
Collaborator

Describe the bug

The lexicographic ordering is applied to a boolean representation of the set, in the form: [b0, b1, b2, b3], where b0 is the lexicographically smallest possible number in the set (from UB) and b3 is the largest. For example, if we have a set variable s that takes its values in [ {}, {0, 1, 2, 3, 4} ], the value {1, 2} is represented as 01100.

The problem is that the lexical ordering of this representation is not equivalent to the lexicographic ordering of the sorted set elements. For example (with the same domain [ {}, {0, 1, 2, 3, 4} ]), if s1 = {4} and s2 = {3}, s1 > s2 according to the lexicographic ordering of the sorted set elements. But with the boolean representation we have: s1 = 00001 and s2 = 00010, thus s1 < s2.

Possible solution

One possible solution could be to reverse the boolean representation, with the previous example we would have:

s1 = {4} -> 10000
s2 = {3} -> 01000

@dimitri-justeau dimitri-justeau changed the title [BUG] Set constrain setLe and setLt do not behave as expected [BUG] Set constraint setLe and setLt do not behave as expected Sep 22, 2022
@cprudhom
Copy link
Member

You are right, I've missed that when I added the decomposition.
The current version of setLt is:

IntIterableRangeSet union = new IntIterableRangeSet();                                                                   
for (int v : a.getUB()) {                                                                                                
    union.add(v);                                                                                                        
}                                                                                                                        
for (int v : b.getUB()) {                                                                                                
    union.add(v);                                                                                                        
}                                                                                                                        
return ref().lexLess(ref().setBoolsView(a, union.size(), union.min()), ref().setBoolsView(b, union.size(), union.min()));

a possible correction could be:

IntIterableRangeSet union = new IntIterableRangeSet();
for (int v : a.getUB()) {
    union.add(v);
}
for (int v : b.getUB()) {
    union.add(v);
}
int offset = union.min();
int size = union.size();
BoolVar[] boolA = new BoolVar[size];
BoolVar[] boolB = new BoolVar[size];
for (int i = 0; i < size; i++) {
    boolA[size - i - 1] = ref().setBoolView(a, i + offset);
    boolB[size - i - 1] = ref().setBoolView(b, i + offset);
}
return ref().lexLess(boolA, boolB);

When evaluated on steiner-systems + steiner_t3_k4_N8 (MiniZinc Challenge 2021), I got:

m = 14;
C = [1..4, {1,2,5,6}, 3..6, {1,3,5,7}, {2,4,5,7}, {2,3,6,7}, {1,4,6,7}, {2,3,5,8}, {1,4,5,8}, {1,3,6,8}, {2,4,6,8}, {1,2,7,8}, {3,4,7,8}, 5..8];
----------
Finished in 2s 134msec.

instead of :

m = 14;
C = [4..7, {3,5,6,8}, {3,4,7,8}, {2,5,7,8}, {2,4,6,8}, {2,3,6,7}, 2..5, {1,6,7,8}, {1,4,5,8}, {1,3,5,7}, {1,3,4,6}, {1,2,5,6}, {1,2,4,7}, {1,2,3,8}];
----------
Finished in 46s 475msec.

For comparison:
JaCoP:

m = 14;
C = [1..4, {1,2,5,6}, {1,2,7,8}, {1,3,5,7}, {1,3,6,8}, {1,4,5,8}, {1,4,6,7}, {2,3,5,8}, {2,3,6,7}, {2,4,5,7}, {2,4,6,8}, 3..6, {3,4,7,8}, 5..8];
----------
Finished in 344msec.

Chuffed:

m = 14;
C = [1..4, {1,2,5,6}, {1,2,7,8}, {1,3,5,7}, {1,3,6,8}, {1,4,5,8}, {1,4,6,7}, {2,3,5,8}, {2,3,6,7}, {2,4,5,7}, {2,4,6,8}, 3..6, {3,4,7,8}, 5..8];
----------
Finished in 241msec.

which indicates that some improvements may be done on this decomposition (but also PropIntersection too).

cprudhom added a commit that referenced this issue Oct 18, 2022
@cprudhom
Copy link
Member

This is not correct.
Consider the following problem (in MiniZinc):

var set of 0..1: a;
var set of 1..2: b;
constraint set_le(a,b);
solve satisfy;

It accepts 11 solutions:

a = 0..1;
b = 1..2;
----------
a = 0..1;
b = 1..1;
----------
a = 0..1;
b = 2..2;
----------
a = 0..0;
b = 1..2;
----------
a = 0..0;
b = 1..1;
----------
a = 0..0;
b = 2..2;
----------
a = 1..1;
b = 1..2;
----------
a = 1..1;
b = 1..1;
----------
a = 1..1;
b = 2..2;
----------
a = {};
b = 1..2;
----------
a = {};
b = 1..1;
----------
a = {};
b = 2..2;
----------
a = {};
b = {};
----------
==========

without the correction only 4 of them are found. With the correction, the following solution is missing:

a = 0..1;
b = 1..1;

Indeed, turned into Boolean variables, the variables are, resp. 011 and 010 and the lex_lt constraint does not hold: 011 <_lex 010.

@dimitri-justeau
Copy link
Collaborator Author

You are right, at first glance, I would say that the problem can only happen when there is a "trailing" zero (as it is the only neutral lexicographic element, and as there can be only one zero in a set). The first idea that comes to me now would be omitting the zero from the Boolean representation, do you think it would be correct?

@dimitri-justeau
Copy link
Collaborator Author

dimitri-justeau commented Oct 18, 2022

Huum, actually this might also be an issue with the definition of lexicographic order over sets with different cardinalities. There is no ambiguity when the sets have the same cardinality, however when the cardinality is not the same, we can either:

I think that we want the third option, BUT it remains to be clear whether zero is a neutral element or not. Thinking twice, is many cases I would not want the solver to tell me that {0, 1} <= {1}, e.g. if the values of the set depict indices of a cost array. We could also offer different options, to have either a shortlex, a lex, a colex, with or without zero as a neutral element.

EDIT: to summarize, I think that MiniZinc is not correct either, or not clear enough about the ordering definition.

@cprudhom
Copy link
Member

Thank you for the insights.
Since the constraint is mainly (only?) here for the MiniZinc challenge, I guess we can open an issue on MiniZinc/libminizinc.
What do you think?

@dimitri-justeau
Copy link
Collaborator Author

Yep, I think it would definitely be worth it! Do you want me to do it and tag you, or the opposite ?

@cprudhom
Copy link
Member

If you have time to do it now, I'll do it. If not, I'll do it on Thursday

@dimitri-justeau
Copy link
Collaborator Author

Okay I'm doing it right now !

@dimitri-justeau
Copy link
Collaborator Author

dimitri-justeau commented Oct 19, 2022

Okay, so I was wrong, MiniZinc effectively uses a lexicographic order with trailing zeros at the right, this is why:

{0, 1} < {1} is equivalent to 010 < 100

So yep, we need to fix this in Choco, and I do not know what is your opinion on this, but I feel like this lexicographic ordering for sets is not suited for some combinatorial uses of sets. It feels suited when the sorted elements of a set form a word, but I feel like the colexicographic and shortlex could be more suited for some usages.

@cprudhom
Copy link
Member

cprudhom commented Oct 19, 2022

I don't see why trailing zeros are needed.
{0, 1} < {1} <=> 01 < 10 is correct too.

So, a reverse argmax on the bits could do the job:

s1 = {0,1}
s2 = {1}
b1=[1,1]
b2=[1,0]

a1 = argmax(rev(b1)) // = 0
a2= argmax(rev(b2)) // = 1

a1 < a2

right?

EDIT: that does not work for empty set though

@dimitri-justeau
Copy link
Collaborator Author

Yes, actually it is not trivial with what is currently available in Choco. Ideally, we could have a channelling constraint (or views) that links a set variable and an array of integers such that the array of integers represents the sorted elements of the set variable.

@cprudhom
Copy link
Member

Here is the decomposition defined in nosets.mzn:

predicate set_lt(var set of int: x, var set of int: y) ::promise_total =
  let {
    set of int: U = ub(x) union ub(y);
    int: l = min(U);
    int: u = max(U);
    array[l..u] of var bool: xb = array1d(l..u, [i in x | i in l..u]);
    var l-1..u: xmax = max(x union {l-1});
    array[l..u] of var bool: yb = array1d(l..u, [i in y | i in l..u]);
    var l-1..u: ymax = max(y union {l-1});
    array[l..u] of var bool: b; 
    constraint forall(i in l..u-1) (
        b[i] = let {var 1..4: cmp = 2*xb[i] + yb[i] + 1} in
                   [b[i+1], xmax < i, ymax > i, b[i+1]][cmp]
    );
    constraint b[u] = (not xb[u] /\ yb[u]);
  } in b[l];

We must compare this decomposition with a one based on views/channeling constraints (and ad hoc prop?).

@dimitri-justeau
Copy link
Collaborator Author

I just coded and tested a quick channelling constraint, I will try it with your test to see if it solves the problem.

@dimitri-justeau
Copy link
Collaborator Author

Okay I proposed a PR here: #954

@cprudhom
Copy link
Member

I integrated your work (with some modifications) in #952 (could you please approve the modifications)?
I first wanted to filter back from ints to set but the ordering involves quite complex rules, so I gave up.

I tested back on steiner-systems, it is now correct and efficient (see below).

Thank you for the fix !

  • {"t": 3, "k": 4, "N": 8}:
m = 14;
C = [1..4, {1,2,5,6}, {1,2,7,8}, {1,3,5,7}, {1,3,6,8}, {1,4,5,8}, {1,4,6,7}, {2,3,5,8}, {2,3,6,7}, {2,4,5,7}, {2,4,6,8}, 3..6, {3,4,7,8}, 5..8];
----------
Finished in 573msec.
  • {"t": 3, "k": 3, "N": 11}:
m = 165;
C = [1..3, {1,2,4}, {1,2,5}, {1,2,6}, {1,2,7}, {1,2,8}, {1,2,9}, {1,2,10}, {1,2,11}, {1,3,4}, {1,3,5}, {1,3,6}, {1,3,7}, {1,3,8}, {1,3,9}, {1,3,10}, {1,3,11}, {1,4,5}, {1,4,6}, {1,4,7}, {1,4,8}, {1,4,9}, {1,4,10}, {1,4,11}, {1,5,6}, {1,5,7}, {1,5,8}, {1,5,9}, {1,5,10}, {1,5,11}, {1,6,7}, {1,6,8}, {1,6,9}, {1,6,10}, {1,6,11}, {1,7,8}, {1,7,9}, {1,7,10}, {1,7,11}, {1,8,9}, {1,8,10}, {1,8,11}, {1,9,10}, {1,9,11}, {1,10,11}, 2..4, {2,3,5}, {2,3,6}, {2,3,7}, {2,3,8}, {2,3,9}, {2,3,10}, {2,3,11}, {2,4,5}, {2,4,6}, {2,4,7}, {2,4,8}, {2,4,9}, {2,4,10}, {2,4,11}, {2,5,6}, {2,5,7}, {2,5,8}, {2,5,9}, {2,5,10}, {2,5,11}, {2,6,7}, {2,6,8}, {2,6,9}, {2,6,10}, {2,6,11}, {2,7,8}, {2,7,9}, {2,7,10}, {2,7,11}, {2,8,9}, {2,8,10}, {2,8,11}, {2,9,10}, {2,9,11}, {2,10,11}, 3..5, {3,4,6}, {3,4,7}, {3,4,8}, {3,4,9}, {3,4,10}, {3,4,11}, {3,5,6}, {3,5,7}, {3,5,8}, {3,5,9}, {3,5,10}, {3,5,11}, {3,6,7}, {3,6,8}, {3,6,9}, {3,6,10}, {3,6,11}, {3,7,8}, {3,7,9}, {3,7,10}, {3,7,11}, {3,8,9}, {3,8,10}, {3,8,11}, {3,9,10}, {3,9,11}, {3,10,11}, 4..6, {4,5,7}, {4,5,8}, {4,5,9}, {4,5,10}, {4,5,11}, {4,6,7}, {4,6,8}, {4,6,9}, {4,6,10}, {4,6,11}, {4,7,8}, {4,7,9}, {4,7,10}, {4,7,11}, {4,8,9}, {4,8,10}, {4,8,11}, {4,9,10}, {4,9,11}, {4,10,11}, 5..7, {5,6,8}, {5,6,9}, {5,6,10}, {5,6,11}, {5,7,8}, {5,7,9}, {5,7,10}, {5,7,11}, {5,8,9}, {5,8,10}, {5,8,11}, {5,9,10}, {5,9,11}, {5,10,11}, 6..8, {6,7,9}, {6,7,10}, {6,7,11}, {6,8,9}, {6,8,10}, {6,8,11}, {6,9,10}, {6,9,11}, {6,10,11}, 7..9, {7,8,10}, {7,8,11}, {7,9,10}, {7,9,11}, {7,10,11}, 8..10, {8,9,11}, {8,10,11}, 9..11];
----------
Finished in 2s 369msec.
  • {"t": 4, "k": 4, "N": 10}:
m = 210;
C = [1..4, {1,2,3,5}, {1,2,3,6}, {1,2,3,7}, {1,2,3,8}, {1,2,3,9}, {1,2,3,10}, {1,2,4,5}, {1,2,4,6}, {1,2,4,7}, {1,2,4,8}, {1,2,4,9}, {1,2,4,10}, {1,2,5,6}, {1,2,5,7}, {1,2,5,8}, {1,2,5,9}, {1,2,5,10}, {1,2,6,7}, {1,2,6,8}, {1,2,6,9}, {1,2,6,10}, {1,2,7,8}, {1,2,7,9}, {1,2,7,10}, {1,2,8,9}, {1,2,8,10}, {1,2,9,10}, {1,3,4,5}, {1,3,4,6}, {1,3,4,7}, {1,3,4,8}, {1,3,4,9}, {1,3,4,10}, {1,3,5,6}, {1,3,5,7}, {1,3,5,8}, {1,3,5,9}, {1,3,5,10}, {1,3,6,7}, {1,3,6,8}, {1,3,6,9}, {1,3,6,10}, {1,3,7,8}, {1,3,7,9}, {1,3,7,10}, {1,3,8,9}, {1,3,8,10}, {1,3,9,10}, {1,4,5,6}, {1,4,5,7}, {1,4,5,8}, {1,4,5,9}, {1,4,5,10}, {1,4,6,7}, {1,4,6,8}, {1,4,6,9}, {1,4,6,10}, {1,4,7,8}, {1,4,7,9}, {1,4,7,10}, {1,4,8,9}, {1,4,8,10}, {1,4,9,10}, {1,5,6,7}, {1,5,6,8}, {1,5,6,9}, {1,5,6,10}, {1,5,7,8}, {1,5,7,9}, {1,5,7,10}, {1,5,8,9}, {1,5,8,10}, {1,5,9,10}, {1,6,7,8}, {1,6,7,9}, {1,6,7,10}, {1,6,8,9}, {1,6,8,10}, {1,6,9,10}, {1,7,8,9}, {1,7,8,10}, {1,7,9,10}, {1,8,9,10}, 2..5, {2,3,4,6}, {2,3,4,7}, {2,3,4,8}, {2,3,4,9}, {2,3,4,10}, {2,3,5,6}, {2,3,5,7}, {2,3,5,8}, {2,3,5,9}, {2,3,5,10}, {2,3,6,7}, {2,3,6,8}, {2,3,6,9}, {2,3,6,10}, {2,3,7,8}, {2,3,7,9}, {2,3,7,10}, {2,3,8,9}, {2,3,8,10}, {2,3,9,10}, {2,4,5,6}, {2,4,5,7}, {2,4,5,8}, {2,4,5,9}, {2,4,5,10}, {2,4,6,7}, {2,4,6,8}, {2,4,6,9}, {2,4,6,10}, {2,4,7,8}, {2,4,7,9}, {2,4,7,10}, {2,4,8,9}, {2,4,8,10}, {2,4,9,10}, {2,5,6,7}, {2,5,6,8}, {2,5,6,9}, {2,5,6,10}, {2,5,7,8}, {2,5,7,9}, {2,5,7,10}, {2,5,8,9}, {2,5,8,10}, {2,5,9,10}, {2,6,7,8}, {2,6,7,9}, {2,6,7,10}, {2,6,8,9}, {2,6,8,10}, {2,6,9,10}, {2,7,8,9}, {2,7,8,10}, {2,7,9,10}, {2,8,9,10}, 3..6, {3,4,5,7}, {3,4,5,8}, {3,4,5,9}, {3,4,5,10}, {3,4,6,7}, {3,4,6,8}, {3,4,6,9}, {3,4,6,10}, {3,4,7,8}, {3,4,7,9}, {3,4,7,10}, {3,4,8,9}, {3,4,8,10}, {3,4,9,10}, {3,5,6,7}, {3,5,6,8}, {3,5,6,9}, {3,5,6,10}, {3,5,7,8}, {3,5,7,9}, {3,5,7,10}, {3,5,8,9}, {3,5,8,10}, {3,5,9,10}, {3,6,7,8}, {3,6,7,9}, {3,6,7,10}, {3,6,8,9}, {3,6,8,10}, {3,6,9,10}, {3,7,8,9}, {3,7,8,10}, {3,7,9,10}, {3,8,9,10}, 4..7, {4,5,6,8}, {4,5,6,9}, {4,5,6,10}, {4,5,7,8}, {4,5,7,9}, {4,5,7,10}, {4,5,8,9}, {4,5,8,10}, {4,5,9,10}, {4,6,7,8}, {4,6,7,9}, {4,6,7,10}, {4,6,8,9}, {4,6,8,10}, {4,6,9,10}, {4,7,8,9}, {4,7,8,10}, {4,7,9,10}, {4,8,9,10}, 5..8, {5,6,7,9}, {5,6,7,10}, {5,6,8,9}, {5,6,8,10}, {5,6,9,10}, {5,7,8,9}, {5,7,8,10}, {5,7,9,10}, {5,8,9,10}, 6..9, {6,7,8,10}, {6,7,9,10}, {6,8,9,10}, 7..10];
----------
Finished in 3s 400msec.

@dimitri-justeau
Copy link
Collaborator Author

dimitri-justeau commented Oct 21, 2022

Nice !!

I also think that the new channelling constraint can be useful in some other situations :-) Yes, I thought about doing the reverse filtering too, I guessed that we might be able to filter a bit more that way, but as this was not really useful for set_le and set_lt (the ints are hidden variables and are only constrained through the channelling constraint), I thought that it wasn't necessary to do it now. We can see that in the future if a use case justifies the investment.

First I wanted to do it with views, but actually, an array of views would have been a bit complicated and would involve a lot of events so I think channelling was a better and simpler option.

cprudhom added a commit that referenced this issue Oct 25, 2022
* Fix issue #936

* Update expected number of solutions (/!\ the fix is not correct though)

* Merge #954

* Merge #954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants