tpaviot / ProcessScheduler

A Python package for automatic and optimized resource scheduling
https://processscheduler.github.io/
GNU General Public License v3.0
58 stars 17 forks source link

How to add first logic constraints with new way #107

Closed dreinon closed 2 years ago

dreinon commented 2 years ago

I noticed a new (and easier) way of adding constraints to the problem was applied. Instead of having to save the constraints into a list and then call problem.add_constraints(list), they get added once they are initialized.

I like it, but Im thinking that, for instance, if I wanted to add a xor constraint, I would first need to initialize two constraints (and thus, they would get added to the problem), and then call ps.xor_([cstr1, cstr2]). After that call, I guess that xor constraint would be added to the problem, but what about cstr1 and cstr2, they would have been added to the problem as single constraints and therefore, the solver will try to satisfy both + xor_.

Is this the intended way of working? Thanks!

dreinon commented 2 years ago

Ping @tpaviot. I think this issue is important since it makes constraints that use first-order logic work badly.

tpaviot commented 2 years ago

@dreinon If you use ps.xor_([cstr1, cstr2]), cstr1 and cstr2become part of a boolean operation, and they won't be evaluated independently from each other. At least that should work like that

dreinon commented 2 years ago

But, when you create them, they get added to the problem, right?

dreinon commented 2 years ago

Then, when you use them in ps.xor for instance, both constraints get removed from the problem constraints and only the xor gets added? https://github.com/tpaviot/ProcessScheduler/blob/2d39619b89f82dc7753c85d788c66f3a38247e1f/processscheduler/constraint.py#L46

dreinon commented 2 years ago

Ohh, as I see, when an operation is created, it is returned so we have to use problem.add_constraint to add it, but in the case of constraints, they get automatically added.

https://github.com/tpaviot/ProcessScheduler/blob/2d39619b89f82dc7753c85d788c66f3a38247e1f/processscheduler/first_order_logic.py#L67

Don't you think this API would be a bit weird to use? Wouldn't it be better to add the constraint inside operation methods?

And would there be any problem if cstr1, cstr2 and xor(cstr1, cstr2) are all added to the problem? Would xor one be stronger than the other two alone?

tpaviot commented 2 years ago

The Constraint class has a new created_from_assertion flag, see https://github.com/tpaviot/ProcessScheduler/blob/master/processscheduler/constraint.py#L37

If such a constraint is part of a first order logic assertion such as or/and/ife then it is evaluated only from this assertion, see https://github.com/tpaviot/ProcessScheduler/blob/64f084b7a1393e8688984388bbe9809a5de2517c/processscheduler/solver.py#L148

tpaviot commented 2 years ago

For example, the following problem conflicts (no solution):

cstr1 = ps.TaskStartAt(task_1, 10)
cstr2 = ps.TaskStartAt(task_1, 20)

whereas the following does not conflict:

cstr1 = ps.TaskStartAt(task_1, 10)
cstr2 = ps.TaskStartAt(task_1, 20)
ps.xor_([cstr1, cstr2])
dreinon commented 2 years ago

Awesome, but we do have to add it manually to the problem right? Wouldn't it be more useful to follow the same notation as with regular constraints and adding them internally to the problem_context.constraints ? I mean:

# Current method
def or_(list_of_constraints: List[Union[BoolRef, Constraint]]) -> BoolRef:
    """Boolean 'or' between a list of assertions or constraints.
    At least one assertion in the list must be satisfied.
    """
    return Or(_constraints_to_list_of_assertions(list_of_constraints))
# Proposed method
def or_(list_of_constraints: List[Union[BoolRef, Constraint]]) -> BoolRef:
    """Boolean 'or' between a list of assertions or constraints.
    At least one assertion in the list must be satisfied.
    """
    return ps_context.main_context.add_constraint(Or(_constraints_to_list_of_assertions(list_of_constraints)))
tpaviot commented 2 years ago

That would not work, because fol constraints can be nested, they must not be added as individual constraints.

Yes, they have to be manually added indeed, I did not finish that part of the job. I have to think about the best way to proceed. By the way, there are not enough unit tests for this part of the library.

dreinon commented 2 years ago

Okay, thank you, that makes sense