Set Expressions (with Chainable Format) for Arithmetic and Relational Operations#1171
Set Expressions (with Chainable Format) for Arithmetic and Relational Operations#1171
Conversation
… existing structure for integer expressions. + update headers Suggestion of improvements: detect constraints like `partition(SetVar[] sets, SetVar universe)`
| return model.allEqual(xSet, ySet); | ||
| case NE: | ||
| return model.allDifferent(xSet, ySet); | ||
| case CONTAINS: |
There was a problem hiding this comment.
Why having both SUBSET and CONTAINS ?
I think I prefer subset
| case CONTAINS: | ||
| return model.subsetEq(ySet, xSet); | ||
| case NOT_CONTAINS: | ||
| return model.disjoint(xSet, ySet); |
There was a problem hiding this comment.
if contains(x, y) means that y is a subset of x
I would expect not_contains(x, y) to mean that y is not a subset of x, which does not mean that both sets are disjoint. They might have one value in common.
There was a problem hiding this comment.
Not necessarily. How would you express the following constraint otherwise ? "x should not contain the values of y ?"
There was a problem hiding this comment.
I would use the term disjoint for that. It is much clearer.
"Should not contain the values of y" is confusing because a value of a set variable is a set.
Let say X has 2 solutions : {2,3} and {2,5,7}. If Y = {2}, then it does not contain the values of X. Yet, I guess it is not what you meant, you want both variables to have disjoint values.
Also, to contain is a directed concept (A contains B is different from B contains A) whereas disjointness is not.
There was a problem hiding this comment.
I see what you mean. Therefore, I agree that DISJOINT should be a better name than NOT_CONTAINS
There was a problem hiding this comment.
I agree as well. When I wrote this term, I was thinking from a user’s perspective. In that context, “contains” and “not contains” are more commonly used, but they introduce ambiguity. Using “subset” and “disjoint” makes the intended behavior much clearer.
| /** | ||
| * Checks if this set is a subset of a given set of integers. | ||
| * | ||
| * @param y the integer values to check. | ||
| * @return a boolean expression representing the subset relationship. | ||
| */ | ||
| default ReExpression subSet(int... y) { | ||
| return new BiReSetExpression(SetOperator.SUBSET, this, this.getModel().setVar(Arrays.stream(y).toArray())); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if this set is equal to a given set of integers. | ||
| * | ||
| * @param y the integer values to compare. | ||
| * @return a boolean expression representing equality. | ||
| */ | ||
| default ReExpression eq(int... y) { | ||
| return new BiReSetExpression(SetOperator.EQ, this, this.getModel().setVar(Arrays.stream(y).toArray())); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if this set does not contain a given set of integers. | ||
| * | ||
| * @param y the integer values to check. | ||
| * @return a boolean expression representing non-membership. | ||
| */ | ||
| default ReExpression notContains(int... y) { | ||
| return new BiReSetExpression(SetOperator.NOT_CONTAINS, this, this.getModel().setVar(Arrays.stream(y).toArray())); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if this set contains a given set of integers. | ||
| * | ||
| * @param y the integer values to check. | ||
| * @return a boolean expression representing membership. | ||
| */ | ||
| default ReExpression contains(int... y) { | ||
| return new BiReSetExpression(SetOperator.CONTAINS, this, this.getModel().setVar(Arrays.stream(y).toArray())); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if this set is different from a given set of integers. | ||
| * | ||
| * @param y the integer values to compare. | ||
| * @return a boolean expression representing inequality. | ||
| */ | ||
| default ReExpression ne(int... y) { | ||
| return new BiReSetExpression(SetOperator.NE, this, this.getModel().setVar(Arrays.stream(y).toArray())); | ||
| } |
There was a problem hiding this comment.
I would have relied on the methods creating the BiReSetExpression from a ArSetExpression, once the SetVar created from y values, instead of relying on new BiReSetExpression() in these methods.
|
|
||
| /** | ||
| * Basic | ||
| * ============= Relacional ============= values and sets |
| * Empty | ||
| */ | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Should not it be a test from TestNG (more especially one of the "1s" category) ?
|
|
||
| model.getSolver().propagate(); | ||
|
|
||
| assertEquals(2, setA.getValue().toArray()[0]); |
There was a problem hiding this comment.
I would finish this test by testing that we have a ContradictionException when trying to remove value 2, as the set should not be empty
|
|
||
| assertArrayEquals(new int[]{1, 2, 3, 4, 5}, setA.getUB().toArray()); | ||
|
|
||
| setA.notEmpty().post(); |
There was a problem hiding this comment.
Is this relevant for the test ?
| assertArrayEquals(new int[]{1, 2, 3, 4, 5}, setA.getUB().toArray()); | ||
| assertArrayEquals(new int[]{1, 2, 3, 4, 5}, setB.getUB().toArray()); | ||
|
|
||
| setA.notEmpty().post(); |
There was a problem hiding this comment.
Is this relevant for the test ?
There was a problem hiding this comment.
NotEmpty is used to prevent setA from being empty, but I don’t think this is very relevant here. The test would produce the same result without NotEmpty.
|
@gadavidd Even if I created the PR and revamp the code a little, this is your work. |
4 participants
This PR takes the work from @gadavidd (#1107) and extracts the 4 impacting commits and places them back on the up-to-date
developbranch.