[Depends on #3701] Add tests for trivial constraints in pyomo/contrib/solver#3703
[Depends on #3701] Add tests for trivial constraints in pyomo/contrib/solver#3703michaelbynum wants to merge 54 commits intoPyomo:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3703 +/- ##
==========================================
- Coverage 89.92% 88.68% -1.25%
==========================================
Files 902 902
Lines 106393 106557 +164
==========================================
- Hits 95679 94505 -1174
- Misses 10714 12052 +1338
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:
|
| return self.var_map[self.v2id] | ||
|
|
||
|
|
||
| class GurobiDirectQuadratic(GurobiDirectBase): |
There was a problem hiding this comment.
Shouldn't this class be named GurobiConsistentQuadratic? In GurobiObserver this is the used name.
There was a problem hiding this comment.
I'm confused. Can you point me to to the place in GurobiObserver you are referring to?
There was a problem hiding this comment.
I was referring to the field opt of the class _GurobiObserver.
There was a problem hiding this comment.
This class does not use the observer. This one is not persistent. Maybe I am missing something?
There was a problem hiding this comment.
Sorry, maybe I wasn’t clear enough. From what I see, in _GurobiObserver you are requesting opt as a GurobiPersistantQuadratic, but this class doesn’t appear to be defined. However, GurobiPersistant inherits from GurobiDirectQuadratic and is what gets passed to _GurobiObserver. So either I’m missing something, or there’s a naming issue.
There was a problem hiding this comment.
Good catch! Thank you. The observer is expecting GurobiPersistent not GurobiPersistentQuadratic. I will fix that.
| self._solver_model.setObjective(gurobi_expr + repn_constant, sense=sense) | ||
|
|
||
|
|
||
| class _GurobiObserver(Observer): |
There was a problem hiding this comment.
Since this class is just calling the "same" functions on the opt field. Why not just inherits Observer in GurobiPersistantSolver?
There was a problem hiding this comment.
It has to be done this way in order for "manual mode" of the persistent solvers to work. If someone calls opt.add_constraint directly on the solver interface, we need the observer to be updated so things stay synchronized.
There was a problem hiding this comment.
You’re probably right, and I might be missing something. However, I don’t see why adding Observer as a superclass of GurobiPersistent wouldn't mean the observer is always updated, since it’s essentially the same as the persistent object.
There was a problem hiding this comment.
The important thing is that the _change_detector methods get called. This makes my head hurt a little. Let me think on it.
There was a problem hiding this comment.
So if we did something like
class GurobiPersistent(GurobiDirectQuadratic, Observer):
def add_constraints(self, cons):
self._change_detector.add_constraints(cons)
Then there would not be a way to call _add_constraints. It would just be an infinte loop. If GurobiPersistent.add_constraints gets called, it would call the _change_detector, and the _change_detector would again call add_constraints on the observer, which is GurobiPersistent.add_constraints.
That is convoluted, but did it make any sense?
There was a problem hiding this comment.
With the current design, this will always happen: the solver calls the detector, which calls the observer, which in turn calls the solver (Opt) again. Or am I missing something?
There was a problem hiding this comment.
If GurobiPersistent.add_constraints gets called, then that will call ModelChangeDetector.add_constraints, which will then call _GurobiObserver.add_constraints, which then calls GurobiPersistent._add_constraints and not GurobiPersistent.add_constraints. Very subtle but important difference there. However, this conversation is making me realize how convoluted this is. Maybe someone has a better idea?
There was a problem hiding this comment.
Okay, understood! One possible fix would be to give all Observer functions a common prefix (e.g., on_*) and make GurobiPersistantSolver a subclass of Observer. We could then reimplement the functions accordingly.
| raise NoSolutionError() | ||
|
|
||
| def load_vars( | ||
| self, vars_to_load: Sequence[VarData] | None = None, solution_id=None |
There was a problem hiding this comment.
Shouldn't we use Optional here?
5 participants
Summary/Motivation:
This PR adds a test for trivial constraints (both feasible and infeasible) to the new solver tests. It also fixes some related bugs.
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: