-
Notifications
You must be signed in to change notification settings - Fork 25
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
Count globals rebased #245
Conversation
@@ -582,3 +585,65 @@ def deepcopy(self, memodict={}): | |||
return Cumulative(*copied_args) | |||
|
|||
|
|||
class GlobalCardinalityCount(GlobalConstraint): | |||
""" | |||
GlobalCardinalityCount(a,gcc): Collect the number of occurrences of each value 0..a.ub in gcc. |
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.
Can a
contain arbitrary expressions, not only pure values? I guess yes, right?
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 do not yet see why we have this upper bound invariant (len(gcc) == ub + 1)
. Whether gcc
is large or small compared to a
does not really matter, len(gcc)
just tells us something about the number of simple cardinality constraints implied by this global one. E.g., a=[1,2,5,4]
, gcc=[0,1,1]
would simply be equivalent to Count(a,0)=0 & Count(a,1)=1 & Count(a,2)=1
. And a=[1,2,5,4]
, gcc=[0,1,1,0,1,1]
would simply be Count(a,0)=0 & Count(a,1)=1 & Count(a,2)=1 & Count(a,3)=0 & Count(a,4)=1 & Count(a,5)=1
. What would be the problem 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.
with the bounds calculations pr that would be possible, as is only values or intvars are allowed
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.
with the bounds calculations pr that would be possible, as is only values or intvars are allowed
Can you elaborate what you mean?
@Wout4 Review finished, let me know what you think! |
I added a change to the bounds_calucation pr to calculate a.ub appropriatly for gcc |
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.
looks good, left 2 readability comments but can also be merged as is.
def value(self): | ||
a, gcc = self.args | ||
gval = [argval(y) for y in gcc] | ||
return all([gval[i] == Count(a,i).value() for i in range(len(gcc))]) |
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 guess you could do all(self.decompose()).value()
? Would be cleaner? or not possible?
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.
Yes that works, changed it!
Count(arr,val) can only be decomposed if it's part of a comparison | ||
""" | ||
arr, val = self.args | ||
return [eval_comparison(cmp_op, Operator('sum',[ai==val for ai in arr]), cmp_rhs)] |
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.
bit clumsy formulation.
why not using 'sum' as in value?
smth like cnt = sum(ai == val for ai in arr); eval_comparison(cmp_op, cnt, cmp_rhs)
?
could be more readable...
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.
test
@@ -582,3 +585,65 @@ def deepcopy(self, memodict={}): | |||
return Cumulative(*copied_args) | |||
|
|||
|
|||
class GlobalCardinalityCount(GlobalConstraint): | |||
""" | |||
GlobalCardinalityCount(a,gcc): Collect the number of occurrences of each value 0..a.ub in gcc. |
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 do not yet see why we have this upper bound invariant (len(gcc) == ub + 1)
. Whether gcc
is large or small compared to a
does not really matter, len(gcc)
just tells us something about the number of simple cardinality constraints implied by this global one. E.g., a=[1,2,5,4]
, gcc=[0,1,1]
would simply be equivalent to Count(a,0)=0 & Count(a,1)=1 & Count(a,2)=1
. And a=[1,2,5,4]
, gcc=[0,1,1,0,1,1]
would simply be Count(a,0)=0 & Count(a,1)=1 & Count(a,2)=1 & Count(a,3)=0 & Count(a,4)=1 & Count(a,5)=1
. What would be the problem here?
def value(self): | ||
a, gcc = self.args | ||
gval = [argval(y) for y in gcc] | ||
return all([gval[i] == Count(a,i).value() for i in range(len(gcc))]) |
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.
Simpler, based on line 601: return all([Count(a, i).value() == argval(v) for i, v in enumerate(gcc)])
|
||
def deepcopy(self, memodict={}): | ||
""" | ||
Return a deep copy of the AllDifferentExceptO global constraint |
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.
GlobalCardinalityCount
instead of AllDifferentExcept0
tests/test_globalconstraints.py
Outdated
bv = cp.boolvar(5) | ||
self.assertTrue(cp.Model(cp.Xor(bv)).solve()) | ||
self.assertTrue(cp.Xor(bv).value()) | ||
|
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.
Do these tests belong here? I'm happy to let them piggy-back, but maybe something went wrong when splitting features / rebasing the branch.
@@ -582,3 +585,65 @@ def deepcopy(self, memodict={}): | |||
return Cumulative(*copied_args) | |||
|
|||
|
|||
class GlobalCardinalityCount(GlobalConstraint): | |||
""" | |||
GlobalCardinalityCount(a,gcc): Collect the number of occurrences of each value 0..a.ub in gcc. |
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.
with the bounds calculations pr that would be possible, as is only values or intvars are allowed
Can you elaborate what you mean?
No description provided.