Support removal of constraints

[funkey]

Useful to pin and unpin variables.

[cmalinmayor]

@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:

• Requiring a new solver to be instantiated when constraints change (current approach, and perhaps the cleanest)
• Defining a 'remove' or 'update' function in the motile.Constraint class that would remove or update all relevant ilpy.Constraints. I don't the the current architecture enables motile.Constraints to keep track of which ILP constraints they spawn, so this might mean larger changes. Here is my current understanding of the process:
  • Each motile Constraint yields an iterable upon instantiation. This was originally an iterable of 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.
  • When motile.Solver.add_constraints() is called, each constraint is initialized and the lower level ilpy.Constraints are added to an ilpy.Constraints object.
  • The 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.

[tlambert03]

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:

https://github.com/funkelab/ilpy/blob/0c52e02feb11dce5ee8282e102c98dcf398f62c2/ilpy/wrapper.pyx#L230-L236

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)

[tlambert03]

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?

[cmalinmayor]

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).

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!

[tlambert03]

Aha! Indeed! Yeah the name overloading can definitely be confusing

[tlambert03]

Maybe we make an internal convention to never use the ilpy types without the full "ilpy.Thing" namespace?

[cmalinmayor]

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.Constraints generated by a particular motile.Constraint. Consider the case where 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. 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.

[cmalinmayor]

I'm not sure if there is a situation where different motile.Constraints would add the same ilpy.Constraint - it depends on how equality of ilpy.Constraint is defined.

[tlambert03]

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

[tlambert03]

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:

1. on the motile side: some constraints could be unique, such that any instance of a given constraint overwrites any previous one. (so, Solver remembers that it had a MaxChildren instance, and knows how to remove the old constraints and add new ones)
2. on the ilpy side: do some introspection of the variables encapsulated by the constraint and remove/replace them. I'm not even sure this can be done, not easily... and probably starts to just fully re-implement sympy.

---

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 ?

[cmalinmayor]

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.