Don't leave dangling refs when removing more than 350 constraints #272
Conversation
src/optlang/interface.py
Outdated
| @@ -1577,6 +1577,7 @@ def _remove_constraints(self, constraints): | |||
| keys = [constraint.name for constraint in constraints] | |||
| if len(constraints) > 350: # Need to figure out a good threshold here | |||
| self._constraints = self._constraints.fromkeys(set(self._constraints.keys()).difference(set(keys))) | |||
| map(lambda co: setattr(co, "problem", None), constraints) # quicker than a loop | |||
There was a problem hiding this comment.
Is it really faster? Map generates a list which also needs to be created in memory. Does the speed difference matter much? Perhaps defining a local partial function offers similar speed if it matters?
There was a problem hiding this comment.
Was about 100x faster in my benchmarks for that single step (100us vs 200ns). However, the full deletion code is slower (~100ms) so it does not make much difference in practice.
Do you mean the partial instead of the lambda?
There was a problem hiding this comment.
Two different ideas here:
setter = partial(setattr, name="problem", value=None)
for co in constraints:
setter(co)or
setter = object.__setattr__
for co in constraints:
setter(co, "problem", None)There was a problem hiding this comment.
If you do the explicit loop
for co in constraints:
co.problem = Noneis probably the most readable as suggested in the issue.
Also how is setter in the second example different from the normal setattr?
There was a problem hiding this comment.
I still have an optlang branch that implemented 80% of explicitly using the observer pattern with setter and getter methods. I never got beyond a proof of concept for GLPK, though :(
|
@Midnighter I simplified it as requested. |
Co-authored-by: Moritz E. Beber <[email protected]>
This fixes the issue illustrated in #271.