Conversation
There was a problem hiding this comment.
Pull request overview
Implements the D‑NMDT and optional T‑D‑NMDT tightening formulations (Beach et al. 2024) for MIP relaxations of univariate squares (x^2) and bilinear products (x\cdot y), and adds a dedicated test suite to validate correctness and relaxation quality.
Changes:
- Added
src/quadratic_approximations/dnmdt.jlimplementing D‑NMDT for bilinear products and D‑NMDT/T‑D‑NMDT for univariate squares (via optional tightening). - Added
test/test_dnmdt_approximations.jlcovering binary expansion correctness, validity/gap bounds, tightening behavior, and comparisons vs HybS. - Wired the new implementation and tests into the package via
include(...)updates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/quadratic_approximations/dnmdt.jl |
Adds D‑NMDT/T‑D‑NMDT modeling code, containers, and constraints for univariate and bilinear approximations. |
src/InfrastructureOptimizationModels.jl |
Includes the new DNMDT implementation file in the module load path. |
test/test_dnmdt_approximations.jl |
Adds a comprehensive test suite for DNMDT formulations (univariate, bivariate, tightening, comparisons). |
test/InfrastructureOptimizationModelsTests.jl |
Includes the new DNMDT test file in the main test runner. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tighten::Bool = true, | ||
| epigraph_depth::Int = max(2, ceil(Int, 1.5 * depth)), | ||
| ) where {C <: IS.InfrastructureSystemsComponent} |
There was a problem hiding this comment.
epigraph_depth defaults to max(2, ceil(Int, 1.5 * depth)), but epigraph depth controls the number of tangent cuts as 2^depth + 1 (see epigraph.jl). This makes the default potentially explode in constraint count for moderate depth values and could cause big runtime/memory regressions. Consider defaulting epigraph_depth to depth (or a small constant), and/or adding a hard cap / warning in the docstring so callers don’t accidentally create tens of thousands of cuts per (name,t).
| z_con = add_variable_container!( | ||
| container, BilinearProductVariable(), C, names, time_steps; meta = meta, | ||
| ) |
There was a problem hiding this comment.
In the univariate x² approximation, the result variable container is created as BilinearProductVariable(). Reusing the bilinear-product variable type for a square approximation is likely to confuse container lookups and increases the risk of meta/key collisions if a caller builds both xy and x² approximations in the same container (especially if they reuse meta). Consider introducing a dedicated variable type for the DNMDT quadratic result (e.g., DNMDTQuadraticVariable) or avoid creating a separate variable container and store the affine expression directly, consistent with other quadratic approximation implementations.
| IOM._add_hybs_bilinear_approx!( | ||
| setup_h.container, MockThermalGen, ["dev1"], 1:1, | ||
| setup_h.x_var_container, setup_h.y_var_container, | ||
| 0.0, 1.0, 0.0, 1.0, depth, HYBS_META, | ||
| ) |
There was a problem hiding this comment.
This testset relies on HYBS_META being defined elsewhere (currently via test_hybs_approximations.jl being included earlier). That makes the file order-dependent and brittle if someone runs this test file in isolation or if the include order changes. Define const HYBS_META = "HybSTest" locally in this file (or inline the string) to remove the hidden dependency.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jd/hybs_impl #39 +/- ##
================================================
+ Coverage 22.25% 24.54% +2.29%
================================================
Files 74 75 +1
Lines 5483 5650 +167
================================================
+ Hits 1220 1387 +167
Misses 4263 4263
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@copilot update this branch on top of main |
this is the implementation of T-D-NMDT model from 23BeachEnhancements of discretization approaches for nonconvex_part2 but keeping the tightening optional since the results in the paper aren't conclusive about performance.