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:
|
|
|
||
|
|
||
| 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... |
| ) | ||
| ) | ||
| ) | ||
| self._add_sos_constraints( |
There was a problem hiding this comment.
Some solvers do not support sos constraint. How about making this optional?
There was a problem hiding this comment.
I think that the derived classes can choose to raise NotImplementedError. I'd rather they do so consciously.
|
LGTM. One minor comment but not that important. |
|
Okay, if tests pass, I think we can merge this. |
There was a problem hiding this comment.
I think this is OK, but there are a few things I keep coming back to.
Some minor comments that would be nice to get in (and quick to address), along with some longer discussions that can (probably) be deferred to subsequent PRs.
I also unresolved a number of discussions so we can find them more readily when we are working on subsequent PRs.
|
|
||
|
|
||
| def handle_var(node, collector): | ||
| collector.variables[id(node)] = node |
There was a problem hiding this comment.
OK - but we ought to see why ComponentSet is slower and try to fix it there.
| def exitNode(self, node, data): | ||
| return collector_handlers[node.__class__](node, self) | ||
|
|
||
| def beforeChild(self, node, child, child_idx): |
There was a problem hiding this comment.
(I mentioned this in a previous comment, so repeating here so it is not lost for future development / reference)
For performance reasons, it may be good to add a BeforeChildDispatcher as well (especially because this collector is specifically looking for leaf nodes).
That said, this improvement can be deferred to a subsequent PR.
There was a problem hiding this comment.
I would prefer to do this as a separate PR.
| import gc | ||
|
|
||
|
|
||
| """ |
There was a problem hiding this comment.
If you either move this before the imports, or make the following change,this string will be picked up by the RTD API docs:
| """ | |
| __doc__ = """ |
There was a problem hiding this comment.
I did not intend this to be user documentation. The docstring for ModelChangeDetector is mean to be the documentation.
| ref_cons, ref_sos, ref_obj = self._referenced_variables[v_id] | ||
| if not ref_cons and not ref_sos and not ref_obj: |
There was a problem hiding this comment.
I realize that I actually suggested the current syntax, but thinking more on it, it might nice to consider (in the future) the following:
if not any(self._referenced_variables[v_id]):(same functionality, possible clearer, and avoids the creation / access / destruction of unnecessary entries in locals(); unknown if saving the intermediate variables is faster / slowed than the call to any())
This idiom can be used here and numerous places below.
| - modified constraint expressions (relies on expressions being immutable) | ||
| - modified objective expressions (relies on expressions being immutable) |
There was a problem hiding this comment.
It is still weird to me that we make the client remember the mapping from Variable / Param values if they need to manually update constraints (e.g., due to fixed variables / Params changing values). This is implicit in this documentation, too: we will update the user on structural changes to the expressions, but not numeric changes ... and if a numeric change causes a structural change, that is (hopefully) caught by the client (if noly they know they need to look for it)?
Can we at least make these mappings public so that the client can reuse them?
There was a problem hiding this comment.
I thought we discussed this? We can expand the API as needed to support this.
There was a problem hiding this comment.
I don't think we can make the mappings public because they should not be modified. Instead we can add methods for clients to request the information they need. I don't think that needs done as part of this PR.
|
Okay, should be good to go, @jsiirola (assuming tests pass). |
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: