feat: Make Literal wrapper of Predicate#398
Conversation
ImkoMarijnissen
changed the title
|
It might be a good idea to briefly describe what has been done, the main idea, and the main change. It would make reviewing easier. |
|
A few thoughts on this:
|
… finding watchers for literals when negated
maartenflippo
left a comment
maartenflippo
left a comment
There was a problem hiding this comment.
As far as I can tell, this feature is not extensively tested. The test cases with bool2int are not sufficient, since they always have the literal be over a 0-1 variable.
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum IntegerVariableEnum { | ||
| DomainId(AffineView<DomainId>), | ||
| Literal(AffineView<Literal>), | ||
| } |
There was a problem hiding this comment.
Can we move this to pumpkin-solver instead? I don't think we need to pollute the core with this?
There was a problem hiding this comment.
Keep in core, but move to different file
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum IntegerVariableEnum { | ||
| DomainId(AffineView<DomainId>), | ||
| Literal(AffineView<Literal>), | ||
| } |
There was a problem hiding this comment.
Can we rename this? We never suffix the type with what kind of type it is. I guess this is done to avoid name clashes with the trait. Perhaps we can call it AnyInteger?
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub enum IntegerVariableEnum { | ||
| DomainId(AffineView<DomainId>), | ||
| Literal(AffineView<Literal>), | ||
| } |
There was a problem hiding this comment.
Why put the affine view inside the variants? We could unwrap that and let any user wrap the enum with an affine view
There was a problem hiding this comment.
At this point, what is the reason for keeping Literal? We can do without it I think? We can simply implement IntegerVariable for Predicate?
| #[derive(Debug, Default, Clone)] | ||
| pub(crate) struct PredicateWatchList { | ||
| /// The watch list from predicates to propagators. | ||
| pub(crate) watch_list_predicate_id: KeyedVec<PredicateId, Vec<PropagatorId>>, | ||
| // TODO: Should use direct hashing | ||
| pub(crate) literal_watch_list: | ||
| HashMap<Literal, HashMap<PropagatorId, (LocalId, EnumSet<DomainEvent>)>>, | ||
| pub(crate) literal_watch_list_backtrack: | ||
| HashMap<Literal, HashMap<PropagatorId, (LocalId, EnumSet<DomainEvent>)>>, |
There was a problem hiding this comment.
Conceptually, why would we have predicates with and without a local ID? Can they be watched as both? Would it not be cleaner to say that a notification happens to a LocalId, removing the need for notify_predicate?
There was a problem hiding this comment.
After a discussion we decided to keep this distinction for now. It may make sense to simplify this in the future to always associate any registration with a local ID, but it is not clear that goes without problems
| pub fn scaled(&self, scale: i32) -> IntExpression { | ||
| let int_expr: IntExpression = (*self).into(); | ||
| int_expr.scaled(scale) | ||
| } | ||
|
|
||
| pub fn offset(&self, offset: i32) -> IntExpression { | ||
| let int_expr: IntExpression = (*self).into(); | ||
| int_expr.offset(offset) | ||
| } |
There was a problem hiding this comment.
This should not happen. A BoolExpression should be just a predicate/literal
3 participants
Related to #375.
This PR aims to move away from having to create a fresh variable for each
Literalby making it a wrapper around aPredicate.Any feedback on this PR is welcome!
TODOs
bool2intand the boolean equals constraint since it has a list ofLiterals as input, which sum up to aDomainId, which means that the scaling trick does not work.notifyinstead ofnotify_predicate.Predicatemultiple times). This breaks some of the invariants that we have for certain propagators, so these should be double-checked.to_integer_variablebeing removed.