Model Observer #3695
Model Observer #3695
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3695 +/- ##
==========================================
+ Coverage 89.27% 89.33% +0.05%
==========================================
Files 896 898 +2
Lines 103687 104291 +604
==========================================
+ Hits 92567 93166 +599
- Misses 11120 11125 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Overall, this is great. I have lots of questions, but very few should prohibit merging this.
|
|
||
|
|
||
| def handle_var(node, collector): | ||
| collector.variables[id(node)] = node |
There was a problem hiding this comment.
This is basically reimplementing a version of ComponentSet. Is there a reason not to re-use that object?
There was a problem hiding this comment.
I have found that doing this manually is faster. That is the only reason. I'm not sure if the difference is enough to justify or not.
There was a problem hiding this comment.
I'll try to get some numbers.
There was a problem hiding this comment.
This seems to be about twice as fast.
There was a problem hiding this comment.
OK - but we ought to see why ComponentSet is slower and try to fix it there.
There was a problem hiding this comment.
I don't disagree, but I don't think that needs to be part of the PR. We can always change the underlying implementation of this later. None of this is exposed to the user.
| elif v.value != _value: | ||
| vars_to_update.append(v) |
There was a problem hiding this comment.
Don't you also have to update a constraint if the var is fixed?
There was a problem hiding this comment.
I think this is up to the observer... Worth a discussion.
There was a problem hiding this comment.
I'm not sure what the best solution is for this. I think we need more use cases. I vote we leave it for now. Here are a couple of examples that make this challenging:
m.x * (m.y + m.z) == 0
m.x.fix(1)
opt.solve(m)
m.x.value = 2
Right now, this works fine for the existing persistent solver interfaces because changing the value of m.x does not change the "structure" of the constraint (i.e., it remains linear). Essentially, we create a "mutable linear coefficient" and update that coefficient when the value of the variable changes. Furthermore, this does not require reconstructing the constraint in any way. However,
(m.y + m.z) ** m.x == 0
m.x.fix(1)
will currently just result in an exception for both gurobi persistent and highs persistent (scip is fine).
There was a problem hiding this comment.
I've been thinking about this more. Maybe we can add some public methods to ModelChangeDetector to provide more information if necessary. For example, we could add a method to get all of the constraints used by a variable (the change detector already has that data).
There was a problem hiding this comment.
@jsiirola and I discussed this further and agreed that expanding the interface was a good solution. I added to the docstring explaining that we can expand the interface, but we are waiting to do so until we have a need.
|
Additional question: who is responsible for verifying that the model does not contain any active components that the Solver/Observer doesn't know how to handle? This appears to assume that the responsibility lies with the Solver, but does that mean that the solver needs to also implement it's own observer to make sure that "unallowable" active components aren't added between solves? That makes me think that maybe the responsibility needs to lie in this observer... |
|
Okay, I did a fairly substantial refactor of this. The I'm still working on adding more tests, but I think this should be ready for review. |
|
I think I am finally done with this PR. |
|
I think that codecov is confused. Coverage should be over 90%. |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Overall, this looks really good. Some minor questions / nits that would be nice to discuss / implement, but nothing that needs to block merging.
| if v in self._referenced_variables: | ||
| raise ValueError(f'Variable {v.name} has already been added') | ||
| self._updates.vars_to_update[v] |= Reason.added | ||
| self._referenced_variables[v] = (OrderedSet(), OrderedSet(), ComponentSet()) |
There was a problem hiding this comment.
The code will be more readable / maintainable if we use a namedtuple here
| if p in self._referenced_params: | ||
| raise ValueError(f'Parameter {p.name} has already been added') | ||
| self._updates.params_to_update[p] |= Reason.added | ||
| self._referenced_params[p] = ( |
There was a problem hiding this comment.
The code will be more readable / maintainable if we use a namedtuple here
| self.objs_to_update = DefaultComponentMap(_default_reason) | ||
| self.observers = observers | ||
|
|
||
| def run(self): |
There was a problem hiding this comment.
Stylistic nit, but would notify() be more descriptive than run()?
| v._lb, | ||
| v._ub, | ||
| v.fixed, | ||
| v.domain.get_interval(), |
There was a problem hiding this comment.
This is OK for now, but:
- will break down as soon as we have solver interfaces that support anything other than simple bounded domains (e.g., this won't work for semi-continuous domains)
get_interval()can be inefficient for custom sets
The long-term solution is to probably maintain a list of known domains and perform change detection on them (much like we do for Params). The short-term solution is probably to document this issue in a comment here.
| reason = Reason.no_change | ||
| if _val != val: | ||
| reason |= Reason.value | ||
| if reason: | ||
| self._updates.params_to_update[p] |= reason |
There was a problem hiding this comment.
Cant this all be simplified to:
| reason = Reason.no_change | |
| if _val != val: | |
| reason |= Reason.value | |
| if reason: | |
| self._updates.params_to_update[p] |= reason | |
| if _val != val: | |
| self._updates.params_to_update[p] |= Reason.value |
| self._referenced_params.pop(p) | ||
| self._params.pop(p) | ||
|
|
||
| def _update_var_bounds(self, v: VarData): |
There was a problem hiding this comment.
Is update the right verb here? Would _record_var_bounds be more descriptive?
| reason |= Reason.sos_items | ||
| self._updates.sos_to_update[c] |= reason |
There was a problem hiding this comment.
As above, can't this be simplified to remove the reason local variable:
| reason |= Reason.sos_items | |
| self._updates.sos_to_update[c] |= reason | |
| self._updates.sos_to_update[c] |= Reason.sos_items |
| self._update_sos_constraints(cons) | ||
| self._updates.run() | ||
|
|
||
| def _update_obj_expr(self, obj: ObjectiveData): |
There was a problem hiding this comment.
As above, should the verb be changed from update to record?
| ] = ComponentMap() # maps objective to (expression, sense) | ||
|
|
||
| # maps constraints/objectives to list of tuples (named_expr, named_expr.expr) | ||
| self._named_expressions: MutableMapping[ |
There was a problem hiding this comment.
Why is this _named_expressions and not _con_named_expressions?
| ObjectiveData, Tuple[Union[NumericValue, float, int, None], ObjectiveSense] | ||
| ] = ComponentMap() # maps objective to (expression, sense) | ||
|
|
||
| # maps constraints/objectives to list of tuples (named_expr, named_expr.expr) |
There was a problem hiding this comment.
I think this implementation is OK for now, but named expressions logically form a tree. By flattening the tree here the observer will end up performing repeated work any time an expression is shared. Maybe not a huge issue for Gurobi, but this will be a bigger deal for things like IDAES / ipopt models.
I would recommend adding a TODO to the code to remind us that eventually we should consider updating the internals to store that hierarchy so that we never need to walk any (sub) expression more than once.
github-project-automation
bot
moved this from Review In Progress
to Reviewer Approved
in Pyomo 6.10
5 participants
Summary/Motivation:
This PR extracts the code for detecting changes in models from the persistent solver interfaces to its own, independent functionality. I will have a separate PR shortly that removes the code from the solver interfaces and updates them to use this code.
Changes proposed in this PR:
observerthat can be used to inform other classes of what changed in a pyomo model.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: