Skip to content

Prune fixed mod_hens GDP choices - #140

Open
bernalde wants to merge 3 commits into
mainfrom
fix/issue-71-mod-hens-gdp
Open

bernalde wants to merge 3 commits into
mainfrom
fix/issue-71-mod-hens-gdp

Conversation

@bernalde

Copy link
Copy Markdown
Member

Summary

  • Prune structurally impossible mod_hens exchanger matches from the active GDP choice set by fixing the shared heat, area, and cost variables to zero and deactivating the corresponding disjunctions.
  • Reduce the default active exchanger decisions exposed to GDPopt from 32 disjunctions, including 20 fixed absent choices, to 12 real exchanger decisions.
  • Add regression tests for the pruned GDP structure and Big-M/Hull transformation support across the documented mod_hens formulations.
  • Regenerate the model-size reports; the active nonlinear constraint counts drop for the affected mod_hens formulations.

Tests run

  • /home/bernalde/.pixi/bin/pixi run pytest tests/test_mod_hens.py -v --tb=short
    Result: 33 passed.
  • /home/bernalde/.pixi/bin/pixi run pytest tests/test_module_imports.py -v --tb=short
    Result: 68 passed.
  • /home/bernalde/.pixi/bin/pixi run test
    Result: 305 passed, 1 skipped.
  • /home/bernalde/.pixi/bin/pixi run lint
    Result: exit 0. Black left 83 files unchanged; critical flake8 reported 0. The broader flake8 report still prints existing repository warnings under --exit-zero; typos completed successfully.
  • /home/bernalde/.pixi/bin/pixi run python generate_model_size_report.py
    Result: completed and updated the generated README/model report counts. Existing Pyomo initialization warnings appeared from unrelated models during report generation.
  • timeout 180s /home/bernalde/.pixi/bin/pixi run python -c 'from gdplib.mod_hens import build_model; from gdplib.benchmark import benchmark; benchmark(build_model(), "gdpopt.enumerate", 60, "/tmp/issue71_enumerate", subsolver="gams", solver_gams="dicopt", solver_profile="gams-local", gams_nlp_solver="ipopth", gams_mip_solver="gurobi", gams_minlp_solver="dicopt", gams_local_minlp_solver="dicopt")'
    Result: command exited 0; GDPopt enumerate returned maxTimeLimit at 60.10 s with 12 disjunctions and incumbent objective 120297.46919329677.
  • timeout 180s /home/bernalde/.pixi/bin/pixi run python -c 'from gdplib.mod_hens import build_model; from gdplib.benchmark import benchmark; benchmark(build_model(), "gdpopt.gloa", 60, "/tmp/issue71_gloa", subsolver="gams", solver_gams="dicopt", solver_profile="gams-local", gams_nlp_solver="ipopth", gams_mip_solver="gurobi", gams_minlp_solver="dicopt", gams_local_minlp_solver="dicopt")'
    Result: command exited 0; GDPopt GLOA returned optimal in 57.72 s with objective 114355.14722766657.

Notes

  • I did not run the full 3600-second benchmark campaign; the local solver probes above used 60-second limits to verify the affected paths without committing generated benchmark artifacts.
  • gdpopt.lbb remains blocked by Pyomo/GDPopt control-flow issues rather than additional mod_hens formulation changes. With the gams-local role profile, LBB routes a relaxed-node MIP to minlp_solver=dicopt, which the GAMS writer rejects as unsuitable for model type mip. With the GAMS/Gurobi role profile, LBB reaches the time limit and then hits the open Pyomo issue GDPopt LBB time-limit path calls stale _get_final_results_object Pyomo/pyomo#3941 (_get_final_results_object on the time-limit path).
  • I also confirmed why gdpopt.enumerate can crash before this patch: Pyomo materializes the full discrete solution list before checking time_limit. Removing fixed absent choices from the active GDP structure keeps mod_hens out of that pathological setup path.

Closes #71

@bernalde

Copy link
Copy Markdown
Member Author

Filed the upstream Pyomo issue for the gdpopt.enumerate timeout/memory behavior:

Pyomo/pyomo#3953

That issue covers the Pyomo-side behavior where gdpopt.enumerate materializes the full discrete solution list before checking time_limit. This PR remains the GDPlib model-side mitigation: pruning structurally fixed absent mod_hens exchanger choices so enumerate sees only the real active decisions.

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maintainer review of head e3a76cc.

The structural change is sound and solution-space-preserving: the base "absent" disjunct enforced exactly the four == 0 constraints that _force_exchanger_absent now applies directly (verified across all seven variants), so fixing the variables and deactivating both disjuncts plus the disjunction for the 20 structurally impossible matches changes nothing feasible while removing dead GDP structure. Identifier index orders ((stg, hot, cold) vs (hot, cold, stg)) all check out against the producing code, and the claimed "33 passed" matches the file's parametrization arithmetic.

Findings:

Blocking: README conflict must be resolved by regeneration — this PR's root README size-table row was generated against the pre-pandemic table; main's table has since been rewritten with new model columns (pandemic, grid, multiperiod_blending). The PR is currently CONFLICTING on README.md (merge-tree confirms README is the only conflicted path; common.py, model_size_report.md, and tests merge cleanly). A textual merge of this PR's row would drop main's new columns. Fix: merge origin/main into the branch, rerun generate_model_size_report.py, and commit the regenerated README.md and gdplib/mod_hens/model_size_report.md — do not hand-resolve the table row. Note PR #148 edits the same "Nonlinear constraints" line; whichever lands second must regenerate again.

Nonblocking: load-bearing statement order — in _force_exchanger_absent, Disjunct.deactivate() fixes the indicator to False, so exchanger_absent[...].deactivate() followed by indicator_var.fix(True) only works because the fix comes after the deactivation (and the fix(False) after the exists-disjunct's own deactivate is redundant). Add a one-line comment noting deactivate's indicator side effect so a future reorder does not silently break the semantics.

Nonblocking: shallow transform assert — the Big-M/Hull test asserts only that no active Disjunction remains. Strengthen it by also asserting the pruned variables stay fixed at 0 after transformation.

Question: pruning invisible in metrics — the body's headline "32 disjunctions to 12" does not appear in committed metrics because generate_model_size_report.py reports report.overall, so the Disjunctions row stays 32 and the 96→76 nonlinear drop comes from fixed-variable reclassification. If active structure is what benchmark consumers need, consider surfacing report.activated (separate issue is fine).

Evidence note: the GLOA optimal 114355.147 and enumerate maxTimeLimit results are author-reported with no committed artifacts; the PR fixes the pre-solve crash path, and enumerate still times out.

Issue linkage: issue #71 still lists GLOA, Enumeration, and LBB (LBB blocked upstream by Pyomo/pyomo#3941). Treat "Closes #71" as Refs #71, or check the boxes explicitly and keep the issue open for LBB.

Summary — Blocking: 1. Nonblocking: 2. Questions: 1. Tests: static verification of the absent-disjunct equivalence across variants and merge-tree conflict analysis; CI is green at this head (6/6 checks), but the PR cannot merge while CONFLICTING. I would not merge this until the blocking issue above is addressed. Posting as COMMENT (author account); the main ruleset requires one approving review from another maintainer.

@bernalde

Copy link
Copy Markdown
Member Author

Addressed the review of head e3a76cc in commits 1272890 (merge) and 5bfed57:

  • readme-regen-conflict (Blocking): merged origin/main into the branch (append-only merge commit), took main's rewritten README table as the base, and re-ran generate_model_size_report.py so the mod_hens row is regenerated inside the new table rather than hand-merged — main's pandemic/grid/multiperiod_blending columns are preserved. gdplib/mod_hens/model_size_report.md reproduced its committed content (already correct). The PR should no longer be CONFLICTING; GitHub was still recomputing merge state when this was posted.
  • deactivate-order-comment: _force_exchanger_absent now documents that Disjunct.deactivate() fixes the indicator to False, so each indicator fix must follow that disjunct's deactivate.
  • strengthen-transform-test: the Big-M/Hull test now captures the pruned match set before transformation and asserts those variables survive transformation still fixed at zero (plus a non-empty guard on the pruned set).
  • activated-metrics (question): agreed the active-structure metric is worth surfacing; left as a candidate follow-up on Refactor mod_hens to fix benchmark issues #71generate_model_size_report.py reports report.overall, so exposing report.activated is a generator change affecting every model's report and belongs in its own PR.

Sequencing note: with this branch now regenerated against current main, PR #148 becomes the one that must regenerate the README table after whichever lands first.

Issue linkage reminder from the review stands: treat Closes #71 as Refs #71 (GLOA/enumeration boxes uncheckable without committed evidence; LBB blocked upstream by Pyomo/pyomo#3941).

Tests: pixi run pytest tests/test_mod_hens.py — 33 passed; black --check clean. CI for the new head was starting when this was posted; the previous head was green (6/6).

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.

Refactor mod_hens to fix benchmark issues

1 participant