Conversation
- Fix stale Returns docstring in model_is_valid: LP, QP, QCP, or NLP - Defer working_obj fetch into the obj_degree is None fallback branch - Add Parameters section to _classify_short_circuit_problem docstring - Add comment in _mip_solver_supports_capability explaining that unknown APPSI solvers fall through conservatively to return False - Add test_short_circuit_mixed_degree_model_routes_to_nlp to cover the case where both has_quadratic_constraints and has_nonquadratic_constraints are True (model with quadratic and cubic constraints must route to NLP) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lowup [codex] Refine MindtPy no-discrete short-circuit routing
|
@jsiirola Addressed in bernalde@888bf23. I merged current I also reran |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3907 +/- ##
==========================================
+ Coverage 89.89% 89.93% +0.03%
==========================================
Files 902 902
Lines 106415 106463 +48
==========================================
+ Hits 95663 95747 +84
+ Misses 10752 10716 -36
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:
|
|
As far as I could see, none of the failed tests had to do with code introduced in this PR. We can restart the tests later to make sure everything is working |
jsiirola
left a comment
jsiirola
left a comment
There was a problem hiding this comment.
Overall, this looks pretty good. There is one case where woul will end up with an unexpected exception because of the order in which things are being processed (you are doing the short circuit handling before you have checked / normalized the objective).
| original_obj = next( | ||
| self.original_model.component_data_objects(ctype=Objective, active=True) | ||
| ) |
There was a problem hiding this comment.
This will raise an exception for models with no objective, because model_is_valid() is called before objective_reformulation(). Further, it will "silently pass" for multiobjective models...?
It might make sense to move the basic Objective validation (e.g., lines 832-849) to the beginning of this function?
There was a problem hiding this comment.
Fixed in ee55f3921. I moved the single-objective normalization into set_up_solve_data(), so MindtPy now rejects multiobjective models before any short-circuit logic and adds a temporary dummy objective for no-objective models. The temporary objective is removed before returning to the user model. I also added regressions for both cases in pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py.
| } | ||
| problem_type_articles = {'LP': 'an', 'QP': 'a', 'QCP': 'a', 'NLP': 'an'} | ||
| obj_degree = MindtPy.objective_polynomial_degree | ||
| if obj_degree is None: |
There was a problem hiding this comment.
Note that obj_degree is None at this point if there is no objective (even though later the objective will be a constant).
I actually don't understand the logic, either: if the obj_degree is None coming in, why/how would re-checking it change the degree?
There was a problem hiding this comment.
Fixed in ee55f3921. After normalizing the objective earlier, MindtPy.objective_polynomial_degree is the cached value used for the short-circuit check, so I removed the redundant obj_degree is None re-check. In this path, None now only means a genuinely non-polynomial objective.
|
|
||
| return 'LP', 'mip', None | ||
|
|
||
| def _mip_solver_supports_capability(self, capability): |
There was a problem hiding this comment.
I think this is OK for the current solver interfaces. However, I believe that the new interfaces (both contrib.appsi and contrib.solver) will be removing the "capability" flags all together. In the future, I think the expectation is to just "try and solve the model", and the solver interface will raise an exception or return an error status if the solver cannot solve the model.
There was a problem hiding this comment.
I left this behavior unchanged in ee55f3921. For the current solver interfaces, the explicit capability checks are still the narrowest way to route continuous QP and QCP models without changing failure semantics. I did not broaden this PR to a "try MIP and fall back on any error" policy. The narrower short-circuit follow-up remains tracked in #3906.
| if len(util_block.objective_list) == 1: | ||
| util_block.objective_polynomial_degree = util_block.objective_list[ | ||
| 0 | ||
| ].expr.polynomial_degree() |
There was a problem hiding this comment.
I think that caching this information is a reasonable idea: polynomial_degree() is convenient, but it requires walking the entire expression to compute it. Repeating that work is probably an unnecessary performance hit. A good follow-on PR would be to remove redundant calls to polynimoal_degree() from MindtPy.
There was a problem hiding this comment.
Agreed. ee55f3921 keeps the cached objective degree in the short-circuit path, but I left the broader migration for a separate follow-up. I opened #3920 to track replacing the remaining MindtPy structural polynomial_degree() checks with QuadraticRepnVisitor, following the discussion in #3874 (comment).
| 'Using NLP solver %s to solve.' % config.nlp_solver | ||
| original_obj = next( | ||
| self.original_model.component_data_objects(ctype=Objective, active=True) | ||
| ) |
There was a problem hiding this comment.
Short-circuit routing now depends on objective metadata before MindtPy has normalized the objective state. This path, together with set_up_solve_data(), still assumes there is exactly one active objective, so models with no objective will still raise unexpectedly and multiobjective models can take the first objective silently. Please validate or normalize the objective once before the short-circuit classification path and then use the cached degree directly here.
There was a problem hiding this comment.
Addressed in ee55f3921. Objective normalization now happens once in set_up_solve_data(), the short-circuit path consumes the cached degree directly, multiobjective models raise early, and no-objective models use a temporary dummy objective that is cleaned up before returning.
| ) | ||
|
|
||
|
|
||
| class TestMindtPyShortCircuitRouting(unittest.TestCase): |
There was a problem hiding this comment.
These routing tests cover the LP, QP, QCP, and NLP branches well, but they do not exercise the objective-normalization edge cases that this short-circuit path now depends on. Please add regressions for both a no-objective model and a multiobjective model so the behavior stays aligned with process_objective() and set_up_solve_data().
There was a problem hiding this comment.
Addressed in ee55f3921. I added test_short_circuit_model_with_no_objective_uses_temporary_dummy_objective and test_short_circuit_multiobjective_model_raises in pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py.
|
Addressed the requested review changes in ee55f3921. The fix normalizes the objective before short-circuit routing, removes the redundant cached-degree re-check, adds regressions for missing-objective and multiobjective models, and keeps the current capability-based QP and QCP routing behavior in place for this PR. Tests run: |
3 participants
Fixes #3906 .
Summary/Motivation:
Based on a comment from @jsiirola in #3861 I went ahead and revisited how we are dealing with a problem that has no discrete variables. Now we check whether the MIP solver (eventually we need to change that nomenclature, but I will leave that to a future PR) can address the problem directly. This would be the case with quadratic programs (QPs) or quadratically constrained programs (QCPs). Although Gurobi can deal with NLPs now, usually it would try to solve them to global optimality, defeating the whole purpose of MindtPy, so we are still passing that down to the NLP (again, I need to change the naming here) solver.
This PR depends on #3861, so it is worth looking into it only after merging that
Changes proposed in this PR:
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: