Skip to content

Move OA selection to mip-nlp solver#320

Merged
jkitchin merged 3 commits into
jkitchin:mainfrom
SECQUOIA:fix/gdp-method-oa-api
Jun 24, 2026
Merged

Move OA selection to mip-nlp solver#320
jkitchin merged 3 commits into
jkitchin:mainfrom
SECQUOIA:fix/gdp-method-oa-api

Conversation

@bernalde

Copy link
Copy Markdown
Contributor

Summary

Closes #319.

This moves OA selection toward the solver-family API described in the issue:

  • adds solver="mip-nlp" as a first-class MINLP decomposition selector
  • adds mip_nlp_method / mip_nlp_options
  • routes mip_nlp_method="oa" and "ecp" through the existing OA implementation
  • keeps gdp_method="oa" as a deprecation-warning compatibility alias
  • leaves gdp_method to describe GDP reformulation choices for the new route

The broader MindtPy-style methods (goa, roa, fp, lp_nlp_bb) are reserved by name and raise NotImplementedError until they are implemented.

Validation

  • PYTHONPATH=python python -m ruff check python/discopt/solvers/mip_nlp.py python/discopt/solver.py python/discopt/modeling/core.py python/tests/test_mip_nlp.py python/tests/test_oa.py
  • PYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_oa.py -q

Note: local commit hooks were skipped because pre-commit is not installed in this environment; the targeted Ruff and pytest checks above pass.

@jkitchin

Copy link
Copy Markdown
Owner

A review highlighted:

  1. python/discopt/solver.py (new mip-nlp route, ~2400) — GDP-reformulation asymmetry between the two "equivalent" OA routes.
    The new solver="mip-nlp" route calls reformulate_gdp(model, method="big-m") before solving, but the deprecated gdp_method="oa" alias does not reformulate (it calls solve_mip_nlp on the raw model, matching the old solve_oa path). So for a model containing disjunctions, the documented-as-equivalent old and new routes produce materially different problems. Worth either reformulating in both routes or documenting that the deprecated alias deliberately preserves the no-reformulation legacy behavior. (reformulate_gdp is a no-op when no GDP constraints exist, so models without disjunctions — including all the new tests — are unaffected, which is why this slipped past the suite.)

Notes (not blocking)
The mip-nlp route warns only about leftover **kwargs; named solve_model options it ignores (e.g. threads, strategy, skip_convex_check) are dropped silently, unlike the AMP route which explicitly enumerates ignored named options. Minor consistency gap.

Can you take a look and let me know when you think this is ready to merge?

@bernalde

Copy link
Copy Markdown
Contributor Author

Commits pushed:

  • f5e57df Address mip-nlp GDP routing review

Main changes:

  • Made the deprecated gdp_method="oa" alias reformulate GDP models with big-m before dispatching to solve_mip_nlp, matching the explicit solver="mip-nlp", mip_nlp_method="oa" route.
  • Updated the deprecation warning to say that gdp_method="oa" is interpreted as big-m for GDP reformulation in this compatibility path.
  • Added MIP-NLP warnings for non-default named solve_model options that the route ignores, including threads, strategy, and skip_convex_check.
  • Added a regression test covering GDP reformulation for both the new MIP-NLP route and the deprecated OA alias.

Tests run:

  • PYTHONPATH=python pytest python/tests/test_mip_nlp.py::test_mip_nlp_and_deprecated_oa_alias_reformulate_gdp -q failed before the fix, then passed after.
  • PYTHONPATH=python pytest python/tests/test_mip_nlp.py -q: 5 passed.
  • PYTHONPATH=python python -m ruff format --check python/discopt/solver.py python/tests/test_mip_nlp.py: passed.
  • PYTHONPATH=python python -m ruff check python/discopt/solver.py python/tests/test_mip_nlp.py: passed.
  • PYTHONPATH=python python -m mypy python/discopt/solver.py python/tests/test_mip_nlp.py: passed.
  • PYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_oa.py -q: 12 passed.
  • PYTHONPATH=python pytest python/tests/test_mip_nlp.py python/tests/test_gdp.py python/tests/test_oa.py -q: 114 passed, 22 deselected.

Comments intentionally not addressed:

  • None.

Remaining risks or follow-up:

  • This keeps the MIP-NLP route on the explicit GDP-to-MIP reformulation path. Native disjunctive or logic-based GDP handling should remain a separate solver/design path rather than being implied by this OA compatibility alias.

@bernalde

Copy link
Copy Markdown
Contributor Author

@jkitchin One deeper design question from #319 that I think is worth settling before we build further on this API: should discopt treat disjunctive solvers as a separate axis from MIP/MINLP solvers?

My current mental model is:

  • gdp_method should answer how a disjunctive model is handled: either reformulate disjunctions into an algebraic MIP/MINLP with big-m, hull, mbigm, or auto, or solve the disjunctive form natively with a logic-based method.
  • solver / mip_nlp_method should answer how the resulting algebraic MIP/MINLP is solved. OA belongs in this category in general: it is a MIP-NLP decomposition method, although the same OA idea can be generalized to disjunctive programs as logic-based OA.
  • Logic-based OA for GDPs should probably live with native disjunctive solving, not as a GDP reformulation option.

This PR chooses the reformulation path for the deprecated alias: gdp_method="oa" means OA solver selection plus big-m GDP reformulation for compatibility. Does that match how you want to separate these axes long term, or would you prefer a different naming split for native disjunctive solvers?

@jkitchin jkitchin merged commit e36d683 into jkitchin:main Jun 24, 2026
6 checks passed

Copy link
Copy Markdown
Owner

Agreed on the two-axis split — that's the contract we'll build on, and I've merged this PR since it's correct and unblocks the API.

  • gdp_method = how a disjunctive model is handled: reformulate to algebraic MIP/MINLP (big-m, hull, mbigm, auto) or solve the disjunctive form natively (logic-based methods).
  • solver / mip_nlp_method = how the resulting algebraic MIP/MINLP is solved (OA, ECP, and the reserved goa/roa/fp/lp_nlp_bb).

The native axis already lives on gdp_method today, which is the proof that this split is the existing convention rather than a new one: gdp_method="loa" dispatches to solve_gdpopt_loa (solver.py:2699). So logic-based OA is already a native-disjunctive option, not a mip_nlp_method. This PR's deprecated gdp_method="oa" → "OA solver + big-M reformulation" is consistent with that — it's reformulation + algebraic OA, not native solving.

Two decisions we'll treat as the agreed contract for follow-ups:

  1. Native + mip-nlp ⇒ raise. A native-disjunctive gdp_method (e.g. loa) combined with solver="mip-nlp" is contradictory (one says "solve natively," the other "reformulate to algebraic and decompose"), so it should raise rather than silently reformulate.
  2. goagloa. goa stays a mip_nlp_method (generalized/global OA on the algebraic MINLP); global logic-based OA (gloa) belongs on the native-disjunctive axis under gdp_method. They must not be aliased to each other.

Captured both in #323 so they aren't lost. Thanks for pushing on the design here.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move OA MINLP selection out of gdp_method

2 participants