Open funkey opened 1 year ago
@funkey I think this is why we didn't originally add divisions to the quickstart guide (#56). We already set the MaxChildren
constraint to 1 and there is no convenient way to update it to 2.
Some options for dealing with this include:
motile.Constraint
class that would remove or update all relevant ilpy.Constraint
s. I don't the the current architecture enables motile.Constraint
s to keep track of which ILP constraints they spawn, so this might mean larger changes. Here is my current understanding of the process:
ilpy.Constraint
or ilpy.Expression
, as shown in the motile.Constraint
base class instantiate function. However, MaxChildren returns an iterable of motile.Expression
which is probably the newer update - might want to update the type strings in the base class accordingly.motile.Solver.add_constraints()
is called, each constraint is initialized and the lower level ilpy.Constraint
s are added to an ilpy.Constraints
object.ilpy.Constraints
class we are using to hold the constraints can add new constraints and clear all existing constraints, but not remove or update. Therefore, we would either need to add this capability to ilpy, or motile would need to perform an "update" by clearing all the constraints and adding back in all the ones that we want to keep, but not the removed ones (or adding back in the same constraints with updated values, in the case of an update).
Thoughts? Perhaps we hack on this soon.However, MaxChildren returns an iterable of motile.Expression which is probably the newer update - might want to update the type strings in the base class accordingly.
MaxChildren.instantiate returning Iterable[Expression]
is consistent with the Iterable[ilpy.Constraint | ilpy.Expression]
type defined in the parent class (it's a narrower type, which is allowed). The base class type annotation suggests that subclasses can return whichever they want. ExpressionConstraint
, for example, returns list[ilpy.Constraint]
. I think it's best to keep the more generic "give me any iterable of epression or constraints" on the base class, and let children determine how they prefer to express it.
This works, by the way, because when the solver adds constraints here, it passes each member of the Constraint.instantiate()
iterator to ilpy.Constraints.add()
, and that lower level function also can accept either an Expression or a constraint:
we would either need to add this capability to ilpy
that seems good to me. In terms of python types ilpy.Constraints
is behaving close to a set[Constraint]
... we could take that interface to it's full python implementation and implement the MutableSet interface on it... meaning you would have
__contains__
, __iter__
, __len__
, add
, discard
, etc... Let me know if that's something you want added to ilpy and I can have a look (@funkey)
actually... that raises one question for me: Do you want to model ilpy.Constraints
as a mutable set or a mutable sequence? (namely: if you add the exact same constraint twice, should it increase the length?). Currently it does:
In [16]: constraints = ilpy.Constraints()
In [17]: c = ilpy.Constraint()
In [18]: len(constraints)
Out[18]: 0
In [19]: constraints.add(c)
In [20]: len(constraints)
Out[20]: 1
In [21]: constraints.add(c)
In [22]: len(constraints)
Out[22]: 2
I suspect that the practical impact of that is negligible in terms of the solver... but that's the current state of things. So, inasmuch as we are reviewing ilpy.Constraints
, do you want it to remain a Sequence? or change to a Set?
However, MaxChildren returns an iterable of motile.Expression which is probably the newer update - might want to update the type strings in the base class accordingly.
MaxChildren.instantiate returning
Iterable[Expression]
is consistent with theIterable[ilpy.Constraint | ilpy.Expression]
type defined in the parent class (it's a narrower type, which is allowed).
We can see in my original comment the source of my confusion :) I thought Expression
was overloaded with implementations in motile and ilpy, the same way Constraint
is, and we were using the motile one since it wasn't explicitly spelled out. But I see that it is spelled out in the imports to be coming from ilpy, and that there is no motile.Expression
!
Aha! Indeed! Yeah the name overloading can definitely be confusing
Maybe we make an internal convention to never use the ilpy types without the full "ilpy.Thing" namespace?
actually... that raises one question for me: Do you want to model
ilpy.Constraints
as a mutable set or a mutable sequence? (namely: if you add the exact same constraint twice, should it increase the length?). Currently it does:In [16]: constraints = ilpy.Constraints() In [17]: c = ilpy.Constraint() In [18]: len(constraints) Out[18]: 0 In [19]: constraints.add(c) In [20]: len(constraints) Out[20]: 1 In [21]: constraints.add(c) In [22]: len(constraints) Out[22]: 2
I suspect that the practical impact of that is negligible in terms of the solver... but that's the current state of things. So, inasmuch as we are reviewing
ilpy.Constraints
, do you want it to remain a Sequence? or change to a Set?
This actually is relevant if we are talking about removing all ilpy.Constraint
s generated by a particular motile.Constraint
. Consider the case where two different motile.Constraint
s, call them MCA
and MCB
, that add the same ilpy.Constraint
--let's call it IC
. We then wish to remove MCA
but leave MCB
. The set version makes this complicated, because removing the one copy of IC
would then invalidate MCB
. The sequence makes it simple - we would actually have ICA
and ICB
and be able to remove ICA
with no issues.
I'm not sure if there is a situation where different motile.Constraint
s would add the same ilpy.Constraint
- it depends on how equality of ilpy.Constraint
is defined.
two different motile.Constraints, call them MCA and MCB, that add the same ilpy.Constraint--let's call it IC. We then wish to remove MCA but leave MCB it depends on how equality of ilpy.Constraint is defined.
yeah, I agree we need to define "sameness" here. If the ilpy constraints are "equivalent" (for example, they both express x > 1
) but have different hash values (ICA
and ICB
are different python objects, as is likely the case if they came from MCA
and MCB
), then we can remove one without touching the other. We could also retain pointers to the parent of a constraint in motile. there's definitely options. Let me have a look at our options in ilpy, and how the backends model it too
let's discuss the slightly higher level of desired outcome before getting too in the weeds about how to implement it in ilpy. I have a question about:
Defining a 'remove' or 'update' function in the motile.Constraint class that would remove or update all relevant ilpy.Constraints.
What syntax exactly would we want here?
solver.add_constraints(MaxParents(1))
solver.add_constraints(MaxChildren(1))
let's say we want to update MaxChildren
. There's a few possibilities we could support (not saying all of these are "good" ... just listing some options):
One option is to say no in-place updating of constraints. You can only add and remove constraints, and you must remove them by pointer. This is explicit and would be very easy to implement.
max_children = MaxChildren(1)
solver.add_constraints(max_children)
# then later:
solver.remove_constraints(max_children)
solver.add_constraints(MaxChildren(2))
Another option that is somewhat nice syntactically, but far more difficult to implement and wades into magical territory:
solver.add_constraints(MaxChildren(1))
# then simply
solver.add_constraints(MaxChildren(2))
This could conceivably be done in at least two:
Solver
remembers that it had a MaxChildren
instance, and knows how to remove the old constraints and add new ones)As a middle ground that doesn't require the user to retain pointers, remove_constraints
could accept either a pointer to an instance of Constraint
or the actual type itself, in which case it would nuke all instances of that class.
class Solver:
def remove_constraints(self, constraints: type[Constraint] | Constraint) -> None:
"""Remove all constraints of type `constraints`, or a specific instance."""
which would be used as:
solver.add_constraints(MaxChildren(1))
# then one more step in the middle:
solver.remove_constraints(MaxChildren)
# before adding
solver.add_constraints(MaxChildren(2))
thoughts @cmalinmayor , @funkey ?
I like option 3. The other use case we had in mind was removing PinConstraints, and I can imagine retaining pointers to different PinConstraints and removing specific ones that are affiliated with specific variables, especially in an interative visualization situation. And if we have the functionality you outlined in the example, it is a short jump to:
solver.add_constraints(MaxChildren(1))
solver.update_constraints(MaxChildren(2))
that behind the scenes does exactly the same thing (removes all MaxChildren and adds the new one after). The update function removing all prior instances of that type might be more confusing for the PinConstraint where having more than one potentially does make sense... but for MaxParents and MaxChildren it is a very convenient wrapper.
Useful to pin and unpin variables.