Set Expressions (with Chainable Format) for Arithmetic and Relational Operations#1171
Set Expressions (with Chainable Format) for Arithmetic and Relational Operations#1171cprudhom wants to merge 5 commits into
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.
|
|
||
| /** | ||
| * 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) ?
|
|
||
| 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 ?
There was a problem hiding this comment.
You're right, this is not relevant. I removed it.
|
@gadavidd Even if I created the PR and revamp the code a little, this is your work. |
|
@cprudhom Sure, sorry for the delay; I had some personal issues, but I am now reviewing the comments and will integrate the required changes in a new commit on this branch. |
|
@cprudhom @ArthurGodet @jgFages I addressed the review comments, but I do not have permission to push to dev_setexpr. I pushed the changes to my fork and opened PR #1196 with the updates. |
|
I think the PR is ready to be merged. |
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.