Skip to content

MBM update (preparation for InfGDP)#133

Merged
pulsipher merged 13 commits into
infiniteopt:masterfrom
dnguyen227:mbm_update_v2
Apr 1, 2026
Merged

MBM update (preparation for InfGDP)#133
pulsipher merged 13 commits into
infiniteopt:masterfrom
dnguyen227:mbm_update_v2

Conversation

@dnguyen227
Copy link
Copy Markdown
Contributor

@dnguyen227 dnguyen227 commented Mar 23, 2026

This PR refactors the MBM implementation to use a unified submodel infrastructure (GDPSubmodel) with cached submodels and infeasible disjunct detection. The
_mini_model + _constraint_to_objective pattern is replaced by create_submodel + prepare_objectives + _raw_M, which separates submodel construction from objective evaluation and enables extension by InfiniteOpt.

EXAMPLE

Given a disjunction with:

  • Y[1]: x <= 2 and x >= 1
  • Y[2]: 0 <= x <= 55

When reformulating Y[1]'s constraints with respect to Y[2]'s feasible region (0 ≤ x ≤ 55):

  • x <= 2 : max(x - 2) → M = 53 (at x = 55)
  • x >= 1 : max(1 - x) → M = 1 (at x = 0)

Before (single M per disjunct pair, no caching):
Each call to _mini_model creates a new submodel from scratch. M values are computed via _constraint_to_objective which sets the objective directly. No
infeasible disjunct detection — unbounded subproblems silently use default_M.

After (per-constraint M, cached submodels, infeasibility detection):

  • create_submodel builds and caches submodels in method.store (one per disjunct)
  • prepare_objectives converts constraints to maximization objectives
  • _raw_M solves and handles infeasible (returns nothing), unbounded (uses default_M + clears NaN start values), and optimal (clamps to ≥ 0) cases
  • Infeasible disjuncts are detected and deactivated with a warning
  • Sets of all-zero M values are enforced globally (no binary variables needed)

Key structural changes:

  • _mini_model → create_submodel + _raw_M (separated concerns)
  • _constraint_to_objective → prepare_objectives (public extension point, dispatch on LessThan/GreaterThan)
  • _replace_variables_in_constraint now uses AbstractDict to handle mixed Number/VariableRef maps (needed by InfiniteOpt extension where parameter functions
    evaluate to Numbers at support points)
  • _MBM struct gains deactivated::Set and store::Dict fields
  • GDPSubmodel struct added to datatypes.jl (shared by MBM and CP)

Relevant tests were added to cover per-constraint M values, infeasible disjunct detection, global constraint detection, and unbounded subproblem fallback.

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.37%. Comparing base (3f167c0) to head (560bdf6).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   99.35%   99.37%   +0.02%     
==========================================
  Files          17       17              
  Lines        1858     1926      +68     
==========================================
+ Hits         1846     1914      +68     
  Misses         12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dnguyen227 dnguyen227 changed the title MBM update (preperatiion for InfGDP) MBM update (preparation for InfGDP) Mar 23, 2026
@dnguyen227 dnguyen227 marked this pull request as ready for review March 23, 2026 15:38
@pulsipher
Copy link
Copy Markdown
Collaborator

@dnguyen227 I am open to arguments otherwise, but I am not sure that I see the utility of deactivating disjuncts. If a constraint is infeasible, we should probably just throw an error. This would simplify things and I am not sure deleting bad disjuncts leads to a useful formulation.

Comment thread src/datatypes.jl Outdated
Comment thread src/datatypes.jl Outdated
Comment thread src/datatypes.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread src/mbm.jl Outdated
@dnguyen227
Copy link
Copy Markdown
Contributor Author

@dnguyen227 I am open to arguments otherwise, but I am not sure that I see the utility of deactivating disjuncts. If a constraint is infeasible, we should probably just throw an error. This would simplify things and I am not sure deleting bad disjuncts leads to a useful formulation.

I overlooked this in my original implementation, below is the paper's call to remove infeasible disjuncts.

"... if (2) is infeasible, then the term ki′ is infeasible and can be removed from the original problem.**"

Where (2) is the subproblem solving for M, and ki' is the disjunct term.

With regard to the utility of actually implementing this. I'm assuming that for the infinite-dimensional case, disjuncts can become infeasible at certain support points while being feasible in others, and this is would be worth the hassle of having code to deactivate disjuncts (at specific support points).

Let me know your thoughts.

@dnguyen227 dnguyen227 requested a review from pulsipher March 31, 2026 18:55
@pulsipher
Copy link
Copy Markdown
Collaborator

@dnguyen227 I am open to arguments otherwise, but I am not sure that I see the utility of deactivating disjuncts. If a constraint is infeasible, we should probably just throw an error. This would simplify things and I am not sure deleting bad disjuncts leads to a useful formulation.

I overlooked this in my original implementation, below is the paper's call to remove infeasible disjuncts.

"... if (2) is infeasible, then the term ki′ is infeasible and can be removed from the original problem.**"

Where (2) is the subproblem solving for M, and ki' is the disjunct term.

With regard to the utility of actually implementing this. I'm assuming that for the infinite-dimensional case, disjuncts can become infeasible at certain support points while being feasible in others, and this is would be worth the hassle of having code to deactivate disjuncts (at specific support points).

Let me know your thoughts.

If I understand correctly, an infeasible problem means the constraints in a disjunct are fundamentally infeasible regardless of the global constraints and other disjuncts. If this is true, then seems like modelling it as such is a fundamental modelling mistake.

@dnguyen227
Copy link
Copy Markdown
Contributor Author

@dnguyen227 I am open to arguments otherwise, but I am not sure that I see the utility of deactivating disjuncts. If a constraint is infeasible, we should probably just throw an error. This would simplify things and I am not sure deleting bad disjuncts leads to a useful formulation.

I overlooked this in my original implementation, below is the paper's call to remove infeasible disjuncts.

"... if (2) is infeasible, then the term ki′ is infeasible and can be removed from the original problem.**"

Where (2) is the subproblem solving for M, and ki' is the disjunct term.
With regard to the utility of actually implementing this. I'm assuming that for the infinite-dimensional case, disjuncts can become infeasible at certain support points while being feasible in others, and this is would be worth the hassle of having code to deactivate disjuncts (at specific support points).
Let me know your thoughts.

If I understand correctly, an infeasible problem means the constraints in a disjunct are fundamentally infeasible regardless of the global constraints and other disjuncts. If this is true, then seems like modelling it as such is a fundamental modelling mistake.

Fair enough. I've removed the deactivation logic and replaced it with an informative error.

Copy link
Copy Markdown
Collaborator

@pulsipher pulsipher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there, just a few minor suggestions.

Comment thread src/datatypes.jl Outdated
Comment thread src/mbm.jl Outdated
Comment thread test/constraints/mbm.jl Outdated
@dnguyen227 dnguyen227 requested a review from pulsipher April 1, 2026 16:48
@pulsipher pulsipher merged commit a7f9eb5 into infiniteopt:master Apr 1, 2026
6 checks passed
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.

2 participants