Conversation
|
Filed the upstream Pyomo issue for the That issue covers the Pyomo-side behavior where |
e6ab9f0 to
e3a76cc
Compare
bernalde
left a comment
There was a problem hiding this comment.
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.
# Conflicts: # README.md
…te order, strengthen transform test
|
Addressed the review of head e3a76cc in commits 1272890 (merge) and 5bfed57:
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 Tests: |
Summary
mod_hensexchanger matches from the active GDP choice set by fixing the shared heat, area, and cost variables to zero and deactivating the corresponding disjunctions.mod_hensformulations.mod_hensformulations.Tests run
/home/bernalde/.pixi/bin/pixi run pytest tests/test_mod_hens.py -v --tb=shortResult: 33 passed.
/home/bernalde/.pixi/bin/pixi run pytest tests/test_module_imports.py -v --tb=shortResult: 68 passed.
/home/bernalde/.pixi/bin/pixi run testResult: 305 passed, 1 skipped.
/home/bernalde/.pixi/bin/pixi run lintResult: 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.pyResult: 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
maxTimeLimitat 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
optimalin 57.72 s with objective 114355.14722766657.Notes
gdpopt.lbbremains blocked by Pyomo/GDPopt control-flow issues rather than additionalmod_hensformulation changes. With thegams-localrole profile, LBB routes a relaxed-node MIP tominlp_solver=dicopt, which the GAMS writer rejects as unsuitable for model typemip. 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_objecton the time-limit path).gdpopt.enumeratecan crash before this patch: Pyomo materializes the full discrete solution list before checkingtime_limit. Removing fixed absent choices from the active GDP structure keepsmod_hensout of that pathological setup path.Closes #71