[codex] Refine MindtPy no-discrete short-circuit routing#1
Conversation
* TXP-7757: removed most deprecation warnings up to current unpublished release * Fixed more warnings * Missing version agnostic functions * Fixed version specific LPStatus enums + renamed get-lb/ub methods * Suggested improvements
Removed the tabulate dependency and replaced it with simple logger output. Fixed _solve_GDP_subproblem so primal_bound is None when the solver fails (not just on preprocessing infeasibility).
[Xpress] Remove deprecation warnings (#1)
bernalde
left a comment
There was a problem hiding this comment.
Overall the classification and routing logic is sound and the new tests are well-structured. The main concern is a subtle overlap in the new constraint lists that, while currently harmless, will mislead readers and could easily cause counting bugs. A few smaller items follow.
All 22 tests pass (including the 6 integration tests against ipopt + appsi_highs). The broader MindtPy suite (78 tests) also passes without regressions.
|
I addressed the actionable review items in a014410. The main behavioral change is that the direct-solve structure cache now uses boolean structural flags instead of overlapping lists, which keeps short-circuit classification independent of |
bernalde
left a comment
There was a problem hiding this comment.
All six items from the first review are addressed. Tests grew from 22 to 25 and all pass, including the integration tests against ipopt + appsi_highs. The broader MindtPy suite (81 tests) also passes without regressions.
The switch from full lists to boolean flags for quadratic/nonquadratic classification is a clean simplification that eliminates the overlap concern entirely. The new test_short_circuit_qcp_detection_ignores_quadratic_strategy test is a good addition that directly validates the interaction between quadratic_strategy=2 and the short-circuit routing.
One minor remaining nit below, otherwise this looks good to go.
|
I did a full pass over this PR and one additional cleanup stood out before it goes upstream: the docstrings around the no-discrete short-circuit path in |
bernalde
left a comment
There was a problem hiding this comment.
The classification and routing logic is sound, the switch from parallel constraint lists to boolean flags is cleaner, and the test suite is well-structured. The items below are what I found after a pass through the final state of the branch.
- 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>
b9fa6fd
into
internal/mindtpy-short-circuit-base
Summary
mip_solveronly when that solver interface explicitly supports the required quadratic structureRoot cause
The no-discrete short-circuit path was doing two things that made future routing work harder:
Impact
This keeps the short-circuit path scoped and predictable while enabling capability-aware routing for continuous quadratic models. Unsupported quadratic cases still go through the NLP solver, and MIP failures are not silently retried with NLP.
Validation
conda run -n base python -m pytest pyomo/contrib/mindtpy/tests/test_mindtpy_no_discrete.py -q