Refactor and update Cumulative and NoOverlap constraints #694
Conversation
| assert is_any_list(end), "end should be a list" | ||
|
|
||
| assert demand is not None, "demand should be provided but was None" | ||
| assert capacity is not None, "capacity should be provided but was None" |
There was a problem hiding this comment.
I don't know what should be done here, but I am not really a fan of giving them a default value (making them optional) and then not allowing the optional value.
There was a problem hiding this comment.
Yeah I also agree it is quite annoying... For now this is the best I could come up with, as you cannot make an argument in the middle optional...
There was a problem hiding this comment.
I am still troubled by this. So, someone either has to actually use the end var after all, or has to use kwargs to define demand and capacity, which are optional but not that optional after all.
Both minizinc (https://docs.minizinc.dev/en/stable/predicates.html) and essence (https://arxiv.org/pdf/2201.03472) actually do not accept end variables at all, and create them for solvers that need them. Should we stick to having them after all?
The question is what if the user needs the end variables for something else I guess; In this case, if the users define themselves the end = start + duration constraints, it is ok. Common subexpression elimination should capture it during the decomposition if needed.
There was a problem hiding this comment.
Hmm yeah, but removing it all together will break every CPMpy model currently out there that uses a Cumulative constraint...
I don't really have too much of an opinion on whether we want to keep the end variables or not, but if we decide to remove them, we should wait a couple of releases and raise a deprecation warning here
IgnaceBleukx
changed the title
|
I used this PR to add some more changes to the Cumulative and NoOverlap constraints.
This PR now also fixes #685 @Dimosts can you re-review these changes? |
Dimosts
left a comment
Dimosts
left a comment
There was a problem hiding this comment.
Mostly approved. See my first comment on end variables and whether we actually want to have them. Not a required change though, I just think it overcomplicates the code if we keep it.
| assert is_any_list(end), "end should be a list" | ||
|
|
||
| assert demand is not None, "demand should be provided but was None" | ||
| assert capacity is not None, "capacity should be provided but was None" |
There was a problem hiding this comment.
I am still troubled by this. So, someone either has to actually use the end var after all, or has to use kwargs to define demand and capacity, which are optional but not that optional after all.
Both minizinc (https://docs.minizinc.dev/en/stable/predicates.html) and essence (https://arxiv.org/pdf/2201.03472) actually do not accept end variables at all, and create them for solvers that need them. Should we stick to having them after all?
The question is what if the user needs the end variables for something else I guess; In this case, if the users define themselves the end = start + duration constraints, it is ok. Common subexpression elimination should capture it during the decomposition if needed.
tias
left a comment
tias
left a comment
There was a problem hiding this comment.
some comments, looks great otherwise
| demand_at_i = demand[i] | ||
| for j in range(len(start)): | ||
| if i != j: | ||
| demand_at_i += demand[j] * ((start[j] <= start[i]) & (end[j] > start[i])) |
There was a problem hiding this comment.
I find this a hard to read style...
I would prefer
demands_at_i = []
for ...
cons.add( demand[i] + cp.sum(demands_at_i) <= capacity )
which is closer to the textbook formula
There was a problem hiding this comment.
I think it needs a bit more explanation, e.g. "only check at the start-time of each task"
and maybe rename 'demand_at_i' to 'demand_with_i' or so
There was a problem hiding this comment.
Updated both the doc and the code to make this more explicit
| demand_at_t += demand * ((start[job] <= t) & (t < end[job])) | ||
| else: | ||
| demand_at_t += demand[job] * ((start[job] <= t) & (t < end[job])) | ||
| demand_at_t += demand[job] * ((start[job] <= t) & (end[job] > t)) |
There was a problem hiding this comment.
like above, please do this as a list instead of the less readable accumulating sum, so that it shows cons.add( cp.sum(demands_at_t) <= capacity )
Or you could actually also make a helper function demands_at(t) and then
for t in ...:
cons.add( cp.sum(demands_at(t)) <= capacity )
not sure what is more readable
There was a problem hiding this comment.
Rewrote it as a single sum. As this is for each time-step, I think it's more natural to write it functionally
Proposal to omit the end variables in scheduling constraints such as Cumulative and NoOverlap.
Makes
endNone by default, and adapts solver behaviour accordingly.It was not possible to make the "dummy" end variables in the constructor, as then they would be added to
user_varsin the solver interface, which is incorrect.Most solvers don't actually need the
endvariables in their interface either, except for OR-Tools.Added a helper-function
get_end_varsto create these dummy end variables.