Skip to content
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

DirectVariable prototype #121

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

DirectVariable prototype #121

wants to merge 54 commits into from

Conversation

tias
Copy link
Collaborator

@tias tias commented May 26, 2022

See #74

A proposal to make native constraints (API calls) more accessible, like we use global constraints.

from cpmpy import *
from cpmpy.expressions.globalconstraints import NativeConstraint
x = intvar(0,10, shape=3)

# CPMpy standard
s = SolverLookup.get("ortools")
s += Table(x, [[0,1,2],[2,1,0]])
print(s.solveAll())

# CPMpy with currently recommended 'direct solver access' use
s = SolverLookup.get("ortools")
s.ort_model.AddAllowedAssignments(s.solver_vars(x), [[0,1,2],[2,1,0]])
print(s.solveAll())

# Proposal in branch
s = SolverLookup.get("ortools")
s += NativeConstraint("AddAllowedAssignments", (x, [[0,1,2],[2,1,0]]))
print(s.solveAll())

# Proposal also allows to indicate some args are const/novar/should be passed as is
# the top two variants (now) also do not scan the data part
s = SolverLookup.get("ortools")
s += NativeConstraint("AddAllowedAssignments", (x, [[0,1,2],[2,1,0]]), arg_novar=[1])
print(s.solveAll())

@IgnaceBleukx
Copy link
Collaborator

Yes, I like this idea a lot.
I am wondering however if we want to support **kwargs in the constructor as well? I think it's more natural to post constraints using just the "native" names of its parameters. arg_novar can then be a list of names instead of indices?

@@ -387,3 +387,31 @@ def deepcopy(self, memodict={}):
"""
copied_args = self._deepcopy_args(memodict)
return Xor(copied_args)

class NativeConstraint(Expression):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to subclass GlobalConstraint for this? Then current transformations will be able to work as is

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because then the decompose() function should be supported, while the idea is that this can only be used directly.

transformations should continue to work though... it should handle any expression (e.g. leave these as is)

@IgnaceBleukx IgnaceBleukx mentioned this pull request Jun 14, 2022
@maxasauruswall
Copy link

Hello All,

I employ the ortools interval constraints quite a bit in my code.

I'd love to use this NativeConstraint to get all my problem's constraints into the cpmpy model.constraints list (we make use of this downstream, and having some constraints only in the ortools solver makes the code messy).

Unfortunately, the ortools interval constraints mix the concept of constraint and variable by requiring that the constraint itself be named (see docs).

Since string values can't be part of the arg_list, even if included in the arg_novar list (because they cannot be reduced to variable expressions, unlike numbers?), I've created a little work-around. I'm wondering if this is an approach folks would consider, or if there is a better way to do this?

Below are screenshots of an example test (I put it as a new case in the test_native.py::TestNativeORTools class), along with the changes to the NativeConstraint class and the ORTools solver method for posting native constraints.

Thanks again for the wonderful package!
Cheers,
Max

image

image

image

@maxasauruswall
Copy link

maxasauruswall commented Jul 29, 2022

Apologies, there is a typo in the above code. The arg_novar param should have been [1], not [size].

@IgnaceBleukx
Copy link
Collaborator

Hi Max,
It might be a good idea indeed to add a **kwargs argument to the NativeConstraint constructor.
@tias we should follow up on this issue as it is a very nice feature to have in the main repo. Apart from deciding on the name, were there any blocking constraint to merge this into master?

@tias
Copy link
Collaborator Author

tias commented Sep 6, 2022

Yes, the name and more examples.

  1. the name:
    I'm currently most in favor of "DirectConstraint" as in, a constraint that is directly posted to the solver through an API call with the same name.
    Or indeed "NativeConstraint" a constraint that has a correspondingly named native API call that will be called.

  2. examples:

I think the IntervalVar of Max is really the missing piece. But the provided example is not enough: it mixes variable and constraint indeed, with as purpose to be used in other constraints (e.g. AddCumulative). So, what do we need to do to support that?

Perhaps we need to add to our ORT object a

def IntervalVar(name=..., ...):
  return s.ort_solver.addIntervalVar(name=....)

meaning that if you do s = SolverLookup("ortools") you can do v = s.IntervalVar(...)
after which, the NativeCosntraint/DirectConstraint could allow:

s += NativeConstraint("AddCumulative", ([v], 2, 4), arg_novar=[1,2])

or so?

If we can think of a clean way to have both NativeConstraint and these intervalvar/optionalvars, then we greatly increase the things we can model in CPMpy (for this and future solvers)

@IgnaceBleukx
Copy link
Collaborator

  1. DirectConstraint or NamedConstraint are my favorites at the moment. Although DirectConstraint might be more google'able friendly...
  2. Adding the AddIntervalVar to the ORT-object would not really help Max in this case as he needs the list of constraints donwstream. To do this, the constraint has to be posted to the model instead of the solver... I think we could make a DirectVar as well which subclasses our Variable class...

@tias
Copy link
Collaborator Author

tias commented Sep 6, 2022

DirectVar is a really neat idea. Because then the model is still separated from the solver object...

Could you work out an example to see if that would work?

@IgnaceBleukx
Copy link
Collaborator

I implemented the DirectVar idea for OR-Tools. It is surprisingly elegant. I only had to make minor changes to get_variables and solver_var.
Added a test for the NoOverlap constraint of OR-tools and it seems to work as intended.
We should add some more documentation to the class DirectVar nevertheless.

@IgnaceBleukx IgnaceBleukx mentioned this pull request Nov 7, 2022
@@ -387,3 +387,31 @@ def deepcopy(self, memodict={}):
"""
copied_args = self._deepcopy_args(memodict)
return Xor(copied_args)

class NativeConstraint(Expression):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because then the decompose() function should be supported, while the idea is that this can only be used directly.

transformations should continue to work though... it should handle any expression (e.g. leave these as is)

@@ -18,8 +18,8 @@

# we only import methods/classes that are used for modelling
# others need to be imported by the developer explicitely
from .variables import boolvar, intvar, cpm_array
from .variables import boolvar, intvar, cpm_array, DirectVar
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for user-facing functions we are using lowercase (e.g. boolvar, intvar) I think I will lowercase this too

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or is this the _impl and should we allow shape=... too

also, there is a difference between the name of a variable and the name of the internal API call...

will think some more about it

@tias
Copy link
Collaborator Author

tias commented Jan 2, 2023

I've did the reverse exercise: how would we want to model with these new capabilities? See 'examples/jobshop_ortools.py' added, a slight modification from jobshop.py, where I use the current 'direct solver access' capabilities of CPMpy to create custom variables and constraints.

Current, variables:

# make intervalvars... ensures start + dur = end
intervals = np.zeros(shape=(machines_count,jobs_count), dtype=object)
for m in all_machines:
    for j in all_jobs:
        intervals[m,j] = model.ort_model.NewIntervalVar(
            model.solver_var(start_time[m,j]), jobs_data[j,m], model.solver_var(end_time[m,j]), f"interval[{m},{j}]")

where we should discuss what we want, for example maximal convenience could be with:

intervals = directvar("NewIntervalVar", [start_time, jobs_data.T, end_time], shape=start_time.shape, name="interval")

where directvar() is a helper function with a shape, that creates an nparray like intvar() does (e.g. of some custom _DirectVariable class)

side-note: in ortools creating these variables ensures that their constraints are ensured. It is a mix of variable and constraint... so we should probably allow to

m += intervals

because people might create intervalvars without using them in future constraints while still wanting the relation between start/size/end to be enforced...

Then, the overlap constraint is with current direct access:

for j in all_jobs:
    model.ort_model.AddNoOverlap(intervals[:,j])

and could become:

for j in all_jobs:
    model += DirectConstraint("AddNoOverlap", intervals[:,j])

Please comment @IgnaceBleukx and others watching this.

@IgnaceBleukx
Copy link
Collaborator

where directvar() is a helper function with a shape, that creates an nparray like intvar() does (e.g. of some custom _DirectVariable class)

That would absolutely be the preferred way of doing things indeed. I hadn't thought of adding a shape arg but it makes perfect sense!

side-note: in ortools creating these variables ensures that their constraints are ensured. It is a mix of variable and constraint... so we should probably allow to

This is very easy to do right? Very similar to something like model += boolvar(). _post_constraint will call solver_var, thereby creating the interval variables on the OR-Tools native solver object (and thus corresponding constraints). We simply need a check at the start of _post_constraint. If and when we decide to go trough with this idea (and we should), this must also become part of the TEMPLATE.py I think.

Now most of the Fuzz-test bugs are fixed, I think this pull request and the bounds-computation issues are highest on the wishlist!

@tias
Copy link
Collaborator Author

tias commented Oct 2, 2023

Converted to (lesser priority) draft. Please say something here if a 'DirectVariable' is what you need, we need a bit more use cases/testing before merging this.

@IgnaceBleukx IgnaceBleukx marked this pull request as ready for review October 31, 2024 13:34
@IgnaceBleukx
Copy link
Collaborator

I have worked some more on this feature, also adding tests for all solvers and some documentation in modelling.md on how to use the directvar.

Should be ready for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants