-
Notifications
You must be signed in to change notification settings - Fork 146
Set Expressions (with Chainable Format) for Arithmetic and Relational Operations #1107
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
base: develop
Are you sure you want to change the base?
Conversation
b2f0ce3
to
020ac80
Compare
Thank you for the PR. |
if (result == null) { | ||
switch (op) { | ||
case SUBSET: | ||
result = model.subsetEq(xSet, ySet).reify().eq(TRUE).boolVar(); |
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.
The expected expression is : (xSet \in ySet) \impl result
but you defined: ((xSet \in ySet) \impl True) \imp result
.
You can simplify the expression to:
result = model.subsetEq(xSet, ySet).reify();
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.
Updated as suggested
solver/src/main/java/org/chocosolver/solver/expression/discrete/set/BiReSetExpression.java
Outdated
Show resolved
Hide resolved
result = model.subsetEq(xSet, ySet).reify().eq(TRUE).boolVar(); | ||
break; | ||
case EQ: | ||
result = model.allEqual(xSet, ySet).reify().eq(TRUE).boolVar(); |
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.
same as l.55
result = model.allDifferent(xSet, ySet).reify().eq(TRUE).boolVar(); | ||
break; | ||
case CONTAINS: | ||
result = model.subsetEq(ySet, xSet).reify().eq(TRUE).boolVar(); |
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.
same as l.55
result = model.allEqual(xSet, ySet).reify().eq(TRUE).boolVar(); | ||
break; | ||
case NE: | ||
result = model.allDifferent(xSet, ySet).reify().eq(TRUE).boolVar(); |
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.
same as l.55
result = model.subsetEq(ySet, xSet).reify().eq(TRUE).boolVar(); | ||
break; | ||
case NC: | ||
result = model.disjoint(ySet, xSet).reify().eq(TRUE).boolVar(); |
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.
same as l.55
import org.chocosolver.solver.variables.SetVar; | ||
|
||
public interface SetExpression { | ||
default SetExpression union(SetExpression y){ |
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 suppose we can have JavaDoc here
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.
Updated as suggested. See if it is in accordance with the added documentation, if anything I can change it again. I tried to follow the standard adopted in the project.
|
||
@Override | ||
public void extractVar(HashSet<IntVar> variables) { | ||
} |
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 would throw an exception here, to be safe
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.
Exception added
|
||
private BoolVar result; | ||
|
||
public UnReSetExpression(SetOperator operator, SetExpression x, int... y) { |
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'm not sure to understand why this class is needed, everything can be done with BiReSetExpression
, no?
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 tried to separate the classes according to responsibility. One for handling the relationship between set and literal value and another for set with set. But I can join them.
* setA.union(setB).post(); | ||
*/ | ||
|
||
public class SetVarExpression extends SetVarImpl implements SetExpression { |
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 this class is useful, can't SetVar implement SetExpression directly?
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 initially considered modifying SetVar
directly, but this approach introduced conflicts due to method inheritance ( getModel()
) and could lead to potential breaking changes. Instead, SetVarExpression
provides a clean abstraction without modifying core components. However, we can consider this change, though it may impact all classes that inherit from SetVar. Would that be a concern?
I initially thought that SetVarExpression
would only replace SetVarImpl
, reducing the overall impact. However, I'm open to whatever is best for choco-solver
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.
Also, I'm sorry, I think it would have been appropriate to have sent you the changes already considering version 5.0.0-beta.1, I will do this in the next corrections.
Set Expressions (with Chainable Format) for Arithmetic and Relational Operations
I've been working with
SetVar
inchoco-Solver
. Unlike other types of variables, such asRealVar
orIntVar
, there was no direct way to compose operations such as equals (eq), not equals (ne), contains, not contains (nc), subset (subSet), union, or intersection directly in the model definition. While propagators allow for constructing such constraints, it was not as intuitive as it is for other variable types likeIntVar
andRealVar
. These features are extremely useful in facilitating the composition of constraints.Thus, I've worked on adding this feature for
SetVar
, based on the operations already available for other variable types. For example:However, such expressions were not possible for
SetVar
:Improvement
I've implemented a new expression framework for
SetVar
, allowing the use of familiar operations directly on set variables. This change introduces support for the following operations:Relational Operations:
eq
(Equals):setA.eq(1, 2, 3)
orsetA.eq(setB)
.ne
(Not Equals):setA.ne(1, 2)
orsetA.ne(setB)
.contains
:setA.contains(1, 2)
.nc
(notContains):setA.notContains(1, 2)
.subSet
:setA.subSet(1, 2, 3)
orsetA.subSet(setB)
.Arithmetic Set Operations:
union
:setA.union(setB)
intersection
:setA.intersection(setB)
This enhancement allows for the expression of complex set relations with simplified syntax, enabling combinations such as:
Example Usage:
Internal Changes:
This change is implemented through three main components:
UnReSetExpression
): Handle relations between a set and a list of values, likesetVar.eq(1,2,3).post()
.BiReSetExpression
): Manage operations between twoSetVar
instances, such assetA.eq(setB).post()
.BiArSetExpression
): Handle arithmetic operations like unions and intersections, e.g.,setA.eq(setB.union(setC)).post()
.A new interface,
SetExpression
, inspired by other Choco-Solver expressions, allowsSetVarExpression
to be used in conjunction with these expression composition objects (UnReSetExpression
,BiReSetExpression
, andBiArSetExpression
).Backward Compatibility:
This improvement is fully backward-compatible with existing constraints and models. It adds considerable flexibility when working with
SetVar
, and I believe it will make modeling with sets much more intuitive.Future Improvements:
I believe that additional operations, such as disjoint, could also be implemented to further enhance set operations in Choco-Solver.
Finally, I've included a comprehensive suite of tests in
SetVarExpressionTest
, which I hope will be useful to validate this enhancement.I would greatly appreciate any feedback or suggestions for optimizations, as well as any edge cases that could further improve this approach!