Repository navigation
Fix #1235 - #1237
Fix #1235#1237
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
|
The modifications on Task should also fix #1119 (even if the issue was created at the time we still used TaskMonitor). |
There was a problem hiding this comment.
In several constraint declarations, you propose moving from a state with two constraints – one of which is reversed – to a state with a single constraint resulting from the merging of the propagators.
Originally, this choice was motivated by improved management of reification: the first constraint is active, whilst only the second (ideally a simple one) is returned to be posted or reified.
I am afraid that the merged versions may lose efficiency:
$c_1 \land (c_2\iff b)$ $(c_1 \land c_2) \iff b$
There was a problem hiding this comment.
The motivation of these changes was to remove the confusion that users can experience when some constraints are posted behind their back (which happened when a constraint was posted while only the second one was returned to be either reified or posted).
If the question is performance, maybe we should run a benchmark on XCSP3 and MiniZinc instances to check whether it had impacts (but maybe these instances do not rely that much on reification ?). Otherwise, we might consider having methods that kept the former way, but personally I don't find it satisfying as it leads to confusion for users
| int[] bounds = VariableUtils.boundsForMultiplication(var1, var2); | ||
| IntVar var4 = ref().intVar(bounds[0], bounds[1]); | ||
| ref().times(var1, var2, var4).post(); | ||
| return arithm(var4, op2, cste); |
There was a problem hiding this comment.
I do not get what is wrong with the previous version
There was a problem hiding this comment.
The idea is to avoid posting constraints behind the back of the user (which could lead to faulty behaviour, most especially when reifying constraints)
There was a problem hiding this comment.
Contrary to the division which removes 0, I do not see which faulty behaviour could come from this. Do you have an example ?
Somehow var4 is created behind the back of the user. I would tend to prefer having the constraint posted because, whether the constraint is posted or reified, var4 is supposed to represent the result of var1 * var2.
Also, var4 would be instantiated whenever other variables are instantiated. Whereas it would remain a free variable if the constraint is neither posted nor reified (which would be a wrong thing to do anyway, just saying).
Therefore, even for the division case - where I understand that removing 0 might cause issue (discussable) - I would tend to prefer the current implementation (var4 is defined as var1 / var2).
There was a problem hiding this comment.
Yeah, for times constraint, as we create the var4 variable for the occasion, I agree it is not necessary to merge the constraints as such. I will change back these cases (but, yes, for the div constraint, posting behind the back of the user might lead to faulty behaviour when using reification)
|
|
||
| public void post() { | ||
| this.getModel().post(new Constraint("Task relation", this)); | ||
| } |
There was a problem hiding this comment.
I have a mixed feeling about this has a Task is also a variable somehow...
Un unposted Task would make no sens to me
@cprudhom this is worth a check for further implications?
There was a problem hiding this comment.
Yes I agree that it might start being weird for Task being constraints and variables. Maybe we need to revamp Task to only be variables ?
That said, the idea behind this change is about reification (more especially for reifying the cumulative constraint).
cprudhom
left a comment
cprudhom
left a comment
There was a problem hiding this comment.
I think we can take this a step further by avoiding any additional constraints on the user’s part (regarding constraints on integers) and by being more clever about how we declare intermediate variables.
Furthermore, there is a suspicious (and, in my view, unnecessary) declaration of an additional variable in count.
|
|
||
| new Propagator[]{ | ||
| new PropKeysorting(vars, SORTEDvars, PERMvars, K)})); | ||
| return new Constraint( |
There was a problem hiding this comment.
We could have used Constraint.merge(...) here
There was a problem hiding this comment.
Would you prefer to ?
| return new Constraint(ConstraintsName.BOOLCHANNELING, new PropEnumDomainChanneling(bVars, var, offset)); | ||
| } else { | ||
| IntVar enumV = var.getModel().intVar(var.getName() + "_enumImage", var.getLB(), var.getUB(), false); | ||
| enumV.eq(var).post(); |
There was a problem hiding this comment.
We must not specify the constraint (or introduce any additional variables).
| } else { | ||
| Model model = value.getModel(); | ||
| IntVar Evalue = model.intVar(model.generateName("COUNT_"), value.getLB(), value.getUB(), false); | ||
| Evalue.eq(value).post(); |
There was a problem hiding this comment.
We must not specify the constraint (or introduce any additional variables).
There was a problem hiding this comment.
The only reason a variable is introduced here is because PropCountVar supposes that the value variable is enumerated. Looking at the code, I can’t think of any particular reasons for this choice.
| return Constraint.merge( | ||
| ConstraintsName.ARITHM, | ||
| ref().div(var1, var2, var4), | ||
| arithm(var4, op2, cste) |
There was a problem hiding this comment.
Depending on the operator, we can avoid using an additional constraint here (and in other cases).
For instance, if op2 equals "GT", we can declare var4 with (cste, bounds[1]).
…ation based on times
341cd99 to
d4062e8
Compare
3 participants
For #1235, the idea is, as suggested in the issue, to return an arithm constraint instead of posting it.
Otherwise, this Pull Request mainly assure that no constraints are posted within factories anymore (unless LCG is activated).